Library_Engine: Update libraries to work with Datasets #1324
Conversation
…tibrary is accessed Also adding method to get out source as well as Dataset
IsakNaslundBh
requested review from
enarhi,
JosefTaylor,
rwemay,
al-fisher,
adecler and
kayleighhoude
There was a problem hiding this comment.
This LGTM, test works well @IsakNaslundBh
Have made a comment on the code, more of a question really, am happy to not hold up the PR based on it.
| } | ||
| catch | ||
| { | ||
| AddDeserialisationEvent(name, oM.Reflection.Debugging.EventType.Warning, "Failed to deserialise at least one item in the dataset named " + name); |
There was a problem hiding this comment.
Would it be helpful to state which entry has failed to deserialise?
There was a problem hiding this comment.
Presume it could be useful, but two points:
- As it failed to deserialise, I will not really have anything to look at for a generic case to state what it was. As in, failing to deserialise means I dont have a BHoMObject to take name or similar from to display. Could paste the whole string out or something like that, but not sure how useful that would be?
- In general when we raise multiples of the same error, it generally feels as it is more clogging the UI than helping out + at least GH has a maximum error count of 15.
Please let me know if you are ok with this, or want me to update!
There was a problem hiding this comment.
I'm happy with that, it makes sense to me, happy to go ahead with merging without changes 😄
|
/azp run BHoM_Engine.CheckInstaller |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@IsakNaslundBh will let you decide to merge based on my review or wait for others or addressing my comment as appropriate, but all checks are happy 😄 |
IsakNaslundBh
deleted the
Library_Engine-#1286-UpdateLibrariesToWorkWithDatasets
branch
Issues addressed by this PR
Closes #1286
Test files
To Test, the following can be placed in the Dataset folder on Appdata. These are not finalised datasets yet, but just contructed for testing:
SectionProperties_DataSets.zip
Simple GH testfile
https://burohappold.sharepoint.com/:f:/s/BHoM/ErnA5fNcgJ9Dohk_eyWUZ7EBR_zd4Lp16Azw_-al3BepCA?e=6FE2Bv
Would also encourage to test that current libraries work as expected and to generate a new one using Dataset to test the functionality.
Changelog
Additional comments