-
Notifications
You must be signed in to change notification settings - Fork 69
Wire up Node::load and Node::save conduit_bin yaml case #1489
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
base: develop
Are you sure you want to change the base?
Wire up Node::load and Node::save conduit_bin yaml case #1489
Conversation
cyrush
left a comment
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 for adding this! Please add a note to the changelog.
Question about detection logic:
Should we check if the json or yaml files exists before parsing to avoid an exception to detect the yaml case?
| } | ||
| // support explicit length 0 in a schema | ||
| else if (fetch_yaml_node_from_object_by_name(yaml_doc, yaml_node, "number_of_elements")) | ||
| else if (! fetch_yaml_node_from_object_by_name(yaml_doc, yaml_node, "number_of_elements")) |
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 for finding this!
| Schema s; | ||
| std::string ifschema = ibase + "_json"; | ||
| std::string ifschema_json = ibase + "_json"; | ||
| std::string ifschema_yaml = ibase + "_yaml"; |
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.
Should we try to look for the file before parsing?
We may not have had the "file exists" helper when the json schema read was first implemented.
| std::string error_message; | ||
|
|
||
| std::string ifschema_json = stream_path + "_json"; | ||
| std::string ifschema_yaml = stream_path + "_yaml"; |
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.
same q about checking if the file exists before trying to parse, to avoid exceptions in the case where we have yaml
| size_t nchildren = children().size(); | ||
| for(size_t i=0; i < nchildren;i++) | ||
| if (nchildren == 0) | ||
| { |
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 for fixing this
| for(size_t i=0; i < nchildren;i++) | ||
| if (nchildren == 0) | ||
| { | ||
| os << "[]" << eoe; |
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.
and thanks for fixing this!
Resolves #1313
Additionally fixes a longstanding bug preventing explicit length 0 in yaml schema
Additionally fixes a longstanding bug where empty objects or lists were not written correctly to yaml schema.
TODO need to understand how to choose to save conduit bin yaml. Will this be a new protocol? Something we prefer? What do we use to make this choice?
TODO need to add tests in
t_conduit_node_save_load.cpp