Add base methods for Schemas and Nodes to handle children with pathli… #523
Conversation
|
Naming-wise, get_child vs. fetch_child is a bit confusing, since there's not a concrete difference between "get" and "fetch". Schema has both a shallow and deep check ("has_child" vs "has_path"), which I think works really well. child vs. descendant might also make sense, though in this case, the child is what the newer method is getting, and "descendant" is a bit cumbersome |
|
I was wondering about the coveralls coverage...turns out I only committed the src/libs, not the tests. d'oh! One sec. |
|
so far this is looking good! I am open to making an API breaking change here to get to a better set of names. Let me mull it a bit more. One way to test this would be to make the change and see if any of the unit tests make assumptions. |
| } | ||
| size_t idx = (size_t)m_schema->child_index(name); | ||
| return *m_children[idx]; | ||
| } | ||
|
|
There was a problem hiding this comment.
we will also need a non-const implementation, I think thats issue the CI is pointing out
There was a problem hiding this comment.
Can do! I expect logic to be identical between the two since it's only a getter. Given that, do you have preference for the implementation? I know the brief way is to have the non-const implementation temporarily const-cast then call the const function, Effective C++ style, but other projects prefer having the two full methods.
There was a problem hiding this comment.
Let's go with two methods, since they are pretty short. Thanks!
|
Looks good! I am going to add some unit tests to look at json + yaml, roundtrip. |
|
Update on my thoughts on naming: Pushing a bit further with testing, it seems we will have to re-plum some operations, like generic sets of nodes, diffs, etc. B/c they are using generic fetches. I'll try to get a unit test that covers most these cases and then we can attack. |
|
@HaluskaR fixing some of the internals will be on my plate -- I am digging in on that, and I'll work on this branch. |
|
@HaluskaR I think I am converging on I started to fix some of the tricker internal changes and a small bug with add_child. |
…(reported by Schema::name() and Schema::path()
|
@HaluskaR I think this might be ready to merge, I updated the change log and added you to the contributors. Do you want to put it through any more of your own testing? |
|
Thank you for adding the tests and all the internals! Looks like all the cases that spring to mind are covered--I'll check while I wait for the last CI to complete and then perform the merge. |
…ke names
This is to open discussion on naming, further tests, gotchas, etc. Still need to add remove methods and other niceties.