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

Add operator/= and operator/ to construct a JSON pointer by appending two JSON pointers #1469

Merged
merged 6 commits into from
Mar 11, 2019

Conversation

garethsb
Copy link
Contributor

@garethsb garethsb commented Jan 31, 2019

Add operator/= and operator/ to construct a JSON pointer by appending two JSON pointers, as well as convenience op/= and op/ to append a single unescaped token or array index. Design inspired by std::filesystem::path, based on feedback on PR #1454.


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite.
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

pboettch and others added 2 commits January 24, 2019 16:46
… two JSON pointers, as well as convenience op/= and op= to append a single unescaped token or array index; inspired by std::filesystem::path
@coveralls
Copy link

coveralls commented Jan 31, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling d183bd0 on garethsb-sony:json_pointer-append into 68ec3eb on nlohmann:develop.

@pboettch
Copy link
Contributor

pboettch commented Feb 1, 2019

FWIW: LGTM

@garethsb
Copy link
Contributor Author

garethsb commented Feb 1, 2019

Thanks, @pboettch. Hopefully @nlohmann approves too. :-)

@nlohmann
Copy link
Owner

Very nice!

@garethsb
Copy link
Contributor Author

garethsb commented Feb 13, 2019

Thanks. How would you feel about any or all of:

  • making is_root public, potentially with a rename to empty for consistency with std::filesystem::path (this would avoid users having to compare vs. default-constructed json_pointer to detect this)
  • adding parent_pointer for consistency with std::filesystem::path::parent_path (parent of empty pointer is empty)

The fact that we have pop_back that returns a value (and no back) bothers me a little, but that's probably not a change to make at this point. Likewise the inconsistency of the name of the private top member function (I believe only used by basic_json::patch); it ought to be front for consistency if it were ever made public. Ah, that's not true, since it returns a json_pointer, not a std::string. Perhaps top_pointer, front_pointer or head_pointer?

@garethsb
Copy link
Contributor Author

@nlohmann I have now added parent_pointer and empty as described, and updated the unit tests. Do you think it would be possible to merge these few json_pointer changes sometime? Thanks.

@garethsb
Copy link
Contributor Author

I think the reported Travis CI failures are not actually related to this PR, am I right?

@nlohmann nlohmann self-assigned this Mar 11, 2019
@nlohmann nlohmann added this to the Release 3.6.0 milestone Mar 11, 2019
@nlohmann nlohmann merged commit c983b67 into nlohmann:develop Mar 11, 2019
@nlohmann
Copy link
Owner

Thanks a lot, and thanks in particular for your patience!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants