Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with deep dependency types #340

Merged

Conversation

IsakNaslundBh
Copy link
Contributor

Issues addressed by this PR

Closes #339

Fixing an issue with the dependency ordering that could occur for some cases where a deeper dependency chain required to be evaluated. Was highlighted when trying to push Bar loads by their own, leading to an error in terms of order where the Bars where pushed before for example Nodes and Sections.
Issue has now been fixed with a tweak to the sorting methodology.

Also, adding a non-generic method for extracting dependency types for a particular type.

Test files

Can be validated with new PushTest in verification solution.

Also, pushing a single BarLoad to Robot, while using BHoM/Robot_Toolkit#506 should work.

Changelog

Additional comments

Changing method to compute a dependency depth and use that for sorting the order of the objects
Validated with @alelom that the changes to the create order is only between types that the order is fine to be switched.
For example, IMaterialProperty vs Constraint6DOF, and that all dependencies are still pushed before any potential host object.

Essentially, there are a couple of orders that all are fine. The previous order was fine, but so is this new one. Hence updating the correct order to new order.
Adding test for the case highlighted in the issue that was the one used to find the problem being fixed in the PR
@IsakNaslundBh IsakNaslundBh added severity:critical No workaround exists. Essential to continue type:bug Error or unexpected behaviour labels Feb 1, 2023
@IsakNaslundBh IsakNaslundBh requested a review from alelom February 1, 2023 10:00
@IsakNaslundBh IsakNaslundBh self-assigned this Feb 1, 2023
Copy link
Member

@alelom alelom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code solves the issue and works well! Reviewed all changes and tests.

@IsakNaslundBh
Copy link
Contributor Author

@BHoMBot check compliance
@BHoMBot check required
@BHoMBot check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Feb 2, 2023

@IsakNaslundBh to confirm, the following actions are now queued:

  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check branch-compliance
  • check dataset-compliance
  • check copyright-compliance
  • check code-compliance
  • check documentation-compliance
  • check project-compliance
  • check core
  • check null-handling
  • check serialisation
  • check versioning
  • check installer
  • check ready-to-merge

@bhombot-ci
Copy link

bhombot-ci bot commented Feb 2, 2023

The check code-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@bhombot-ci
Copy link

bhombot-ci bot commented Feb 2, 2023

The check documentation-compliance has already been run previously and recorded as a successful check. This check has not been run again at this time.

@IsakNaslundBh IsakNaslundBh merged commit bce49f9 into develop Feb 3, 2023
@IsakNaslundBh IsakNaslundBh deleted the BHoM_Adapter-#339-FixIssueWithDeepDependencyTypes branch February 3, 2023 07:00
@bhombot-ci bhombot-ci bot mentioned this pull request Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
severity:critical No workaround exists. Essential to continue type:bug Error or unexpected behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BHoM_Adapter: Issue with some deep dependency types
2 participants