Serialiser_Engine: Fix issue with classmaps of inner generic types #2724
Conversation
…gistered outside the compute method
There was a problem hiding this comment.
I am happy with the solution proposed here. Fixes the problem nicely without creating other issues (that I could find).
Only thing I would change is the name of the class from BHoMDefaultClassMapConvention to something like GenericClassMapConvention to make it clearer what it does.
Yes, happy to change to that. Called it something like that first, but change the name when I realised that this really could be applied to all classes, but agree that with current setup that name is better. Will update! |
Need to find a more proper way of testing this that ensures the reversed order of classmap serialisation is guarantiued to always work. UnitTest is to fragile and might give a false sense of security
Changing strategy to use the new BHoMDefaultClassMapConvention for all classmaps and puting it in the default BHoM pack. The settings are also removed from the compute method and now handled by the convention for all cases instead.
|
@IsakNaslundBh to confirm, the following checks are now queued:
|
|
@adecler found some issues with some objects, namely the system DataTable class on the BHoM Table class when I applied the class map to everything. Hence added in the class map convention that it should only be applied to IObjects. Let me know if you think this is not enough. What we can do is to keep this new class map convention as is, but also keep the settings in the compute class. It will ofc be doubling up things, but might be a way out. I am also happy to revert this PR to what it was initially, just fixing the inner generic types, if you think that is better. |
|
@IsakNaslundBh to confirm, the following checks are now queued:
|
|
@IsakNaslundBh , let have another catch-up on this. |
Problem seem to stem from different versions of DataTable used by Engine and Test_Toolkit (netstandard vs framework 4.7.2) and does not have anything to do with Serialisation of the objects themselves. Need to find a proper fix for it, and that is not to filter by IObject here.
1 similar comment
|
@IsakNaslundBh to confirm, the following checks are now queued:
There are 25 requests in the queue ahead of you. |
|
@BHoMBot check required |
|
@IsakNaslundBh to confirm, the following checks are now queued:
|
|
The check |
|
The check |
|
@FraserGreenroyd to confirm, the following checks are now queued:
There are 3 requests in the queue ahead of you. |
There was a problem hiding this comment.
From my understanding of discussion with @IsakNaslundBh this is ok to merge today to unblock work on migrating stuff from Reflection_Engine to Base_Engine, following this mornings conversation between @IsakNaslundBh and @adecler .
The checks are happy with this PR on serialisation, so further eyes will be kept out on changes there. @adecler if you could check over the files changed from @IsakNaslundBh last commit, and if there are any issues with them, please raise new issues so they can be tackled ASAP.
Thanks both!
|
@BHoMBot check ready-to-merge |
|
@FraserGreenroyd to confirm, the following checks are now queued:
There are 7 requests in the queue ahead of you. |
|
FAO: @FraserGreenroyd The check they wish to have dispensation on is documentation-compliance. If you are providing dispensation on this occasion, please reply with:
|
|
FAO: @FraserGreenroyd The check they wish to have dispensation on is code-compliance. 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. 4760430000 |
|
@BHoMBot this is a DevOps instruction. I am authorising dispensation to be granted on check ref. 4760429546 |
|
@FraserGreenroyd I have now provided a passing check on reference |
|
@FraserGreenroyd I have now provided a passing check on reference |
FraserGreenroyd
deleted the
Serialiser_Engine-FixClassMapsOfInnerGenericObejcts
branch
@FraserGreenroyd , I am happy with the last commit. |
Issues addressed by this PR
Closes #2722
Adding ClassMapConvetion to correctly set properties of class maps registered outside the compute method.
See issue for more details as to why this is required.
Could also have a think if we should change how those settings are handled in general by removing all the settings in the compute method
RegisterClassMapand change it so that all objects make use of this added ClassMapConvention instead, to ensure that all objects get these settings, independent on what registered their class map.Would be a quite small change code wise, but tried to disrupt as little as possible with this PR.
@adecler if you have any other suggestion for how this can be handled better, I would be up to changing what ever is needed.
EDIT: After chat with @adecler decided that updating the code so the convention is applied to every single class map gives a cleaner solution. By doing this, the same calls made in the compute method is now redundant and has hence been removed.
Test files
https://burohappold.sharepoint.com/:f:/s/BHoM/EkaZJA6Qzr1Krl472i5JiWEB0wzyDhfPmrKhI200c0EFvQ?e=Jgm5LR
Changelog
Additional comments
🤦♂️ forgot the issue number in the branch name. If this is a problem I will re-raise..