-
Notifications
You must be signed in to change notification settings - Fork 15
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
Serialiser_Engine: fix for MessageForDeleted to be picked up for properties of types that have multiple matching types #3371
Serialiser_Engine: fix for MessageForDeleted to be picked up for properties of types that have multiple matching types #3371
Conversation
@pawelbaran to confirm, the following actions are now queued:
|
@BHoMBot check required |
@pawelbaran to confirm, the following actions are now queued:
|
The check |
The check |
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.
Thanks @pawelbaran ,
The change makes sense. As discussed, I would just add a warning message for the case where multiple matching types where found to still provide good traceability:
Thanks @adecler for the review, I have now addressed the comment 👍 |
@BHoMBot check required |
@pawelbaran to confirm, the following actions are now queued:
|
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.
The corrections are matching the request change so happy to approve.
As a side note, the if-else sequence is a bit weird here. After ruling out count == 1
, there is still a check for count > 1
but only when takeFirstIfMultiple
is true. The final case, will just assume there is more than one type. This is correct since the access to the BHoMTypeDictionary
would have returned false if not types were found. So no bug or error and it was like that before this PR, just a little confusing. So still happy to approve.
@pawelbaran to confirm, the following actions are now queued:
|
Yeah I noticed that too and it tempted me a lot to refactor. I forced myself not to, mainly to avoid scope creep and extra review/testing resource to close out this PR. |
@BHoMBot check unit-tests |
@pawelbaran to confirm, the following actions are now queued:
There are 1 requests in the queue ahead of you. |
FAO: @FraserGreenroyd The check they wish to have dispensation on is unit-tests. If you are providing dispensation on this occasion, please reply with:
|
@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 27324447978 |
@pawelbaran I have now provided a passing check on reference |
Issues addressed by this PR
Closes #3370
Test files
Changelog
Additional comments