-
Notifications
You must be signed in to change notification settings - Fork 4
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
File_Engine: added Compute JSON Write and Read methods #130
Conversation
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.
Looks good in general, however I have one concern on top of @FraserGreenroyd's comments: I think the file extension constraint requiring .json is too strict:
- it will not allow to save
ISettings
to .cfg file - it may not allow valid files with uppercase extension (easy to fix with
ToLower
method)
Co-authored-by: Fraser Greenroyd <[email protected]>
Added
Cfg files have their own syntax depending on context and require a different treatment. Happy to discuss. I would consider that a completely separate feature from this. This still allows to write |
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 CSProj change doesn't appear to have been pushed? The CSProj file will also need other compliance settings set, so might be easier to get BHoMBot to do it?
Still unsure why we have a ToJsonArray
here as well as in Serialiser_Engine?
Also, documentation is missing for the new methods, but I'll run the bot's compliance to give a report of exactly what's needed 😄
I agree with the discussion on Cfg being it's own problem - happy to discuss solutions for that that work for the use cases we have.
@BHoMBot check compliance |
@FraserGreenroyd to confirm, the following checks are now queued:
|
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
Co-authored-by: Fraser Greenroyd <[email protected]>
@BHoMBot check compliance |
@FraserGreenroyd to confirm, the following checks 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.
This LGTM now, thanks @alelom 😄 will let @pawelbaran do a review too before merging
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.
LGTM 🔥
@pawelbaran to confirm, the following checks are now queued:
|
Issues addressed by this PR
Closes #129
Test files
https://burohappold.sharepoint.com/:f:/s/BHoM/Eh96SGBibiBItHu2SmNb2-QBQw2le3Fj4lntIk4FCnoYMQ?e=j3Hrpq