Skip to content
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

Switch to yaml-cpp #2

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Switch to yaml-cpp #2

wants to merge 5 commits into from

Conversation

ksiero
Copy link
Member

@ksiero ksiero commented Mar 18, 2021

  • using yaml-cpp
  • C++11 required
  • new enum PC_tree_type_t that describe node type
  • slight API change (C and Fortran):
    • removed PC_root (was depending on libyaml)
    • added PC_type (returns type of the node)
    • added PC_document_line (returns line node in yaml document)
    • PC_sget is now visible for C API (needed to move it because of g_error_context)
  • quick guide in README.md
  • full documentation
  • tests of new feature

Should be tagged as 0.5.0

@ksiero ksiero marked this pull request as draft March 23, 2021 11:19
@ksiero ksiero marked this pull request as ready for review March 23, 2021 15:10
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is some great work! A lot of it :) didn't have time to review it all yet, but these are my current comments

paraconf/CMakeLists.txt Outdated Show resolved Hide resolved
paraconf/include/paraconf.h Show resolved Hide resolved
paraconf/include/paraconf.h Show resolved Hide resolved
`some_map.yaml`:
```yaml
key_1: value_1
inner_sequence: !include "some_sequence.yaml"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you support the !include syntax in paraconf or was it already in yaml-cpp?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is paraconf feature

*
* \return postprocessed node
*/
PC_node postprocess_result(PC_node node) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the include mechanism would better be implemented in a separate library

paraconf/include/paraconf/PC_node.h Outdated Show resolved Hide resolved
paraconf/src/PC_node.cxx Outdated Show resolved Hide resolved
paraconf/src/PC_node.cxx Outdated Show resolved Hide resolved
size_t PC_node::size() const
{
if (!m_node.IsDefined()) {
throw Error{PC_NODE_NOT_FOUND, "Cannot get size of not defined tree"};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't use defensive programing, just state it as a prerequisite

paraconf/include/paraconf/PC_node.h Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants