-
Notifications
You must be signed in to change notification settings - Fork 6
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 distinct causes assigned properties to be lost #355
Fix distinct causes assigned properties to be lost #355
Conversation
…management First test is failing, second test is not, but added to ensure changes does not negatively impact ID management
…ties modules, ensuring all properties are properly copied across
@BHoMBot check compliance |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@BHoMBot check compliance |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@BHoMBot check unit-tests |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@BHoMBot check unit-tests |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@BHoMBot check unit-tests |
@IsakNaslundBh to confirm, the following actions are now queued:
|
@BHoMBot check unit-tests |
@IsakNaslundBh to confirm, the following actions are now queued:
|
Also adding the steel materials to the input objects for obejcts to be pushed in the DuplicateObjects_EnsureAllOutputHaveIds test. They should have been there in the first palce, but got missed when writing the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving based on review of code changes and unit-tests. The code changes make sense and effectively fix a problem introduced by the "push at the same time" feature. Unit tests cover this fix well.
@IsakNaslundBh to confirm, the following actions are now queued:
There are 34 requests in the queue ahead of you. |
The check |
The check |
The check |
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check versioning |
@FraserGreenroyd to confirm, the following actions are now queued:
|
2 similar comments
@FraserGreenroyd to confirm, the following actions are now queued:
|
@BHoMBot check ready-to-merge |
1 similar comment
@BHoMBot check ready-to-merge |
@FraserGreenroyd to confirm, the following actions are now queued:
|
Issues addressed by this PR
Closes #126
Finally fixing this old issue of better handling merging of properties on "same" object being pushed in, most common case being highlighted in the issue (Node with and without constraints).
Done by instead of call to Distinct, a call to GroupBy is made, after CopyBHoMProperties and any CopyPropertiesModules is called, making sure all relevant properties are merged over.
Also, update to how the ID assignment from any duplicate objects is made, as simply can rely on the group, rather then trying to re-fetch from the list based on another call with the comparer. This should be a bit more efficient than previous implementation.
Test files
Have added new PushTest doing exactly what is highlighted in the issue. Was failing before, but working after changes.
Also, added a new test to check that all pushed objects have ids assigned. Was working before as well, but wanted to add this additional case as I made some changes to how it was handled.
Changelog
Additional comments