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

JSON-pointer: add operator+() returning a new json_pointer #1454

Closed
wants to merge 1 commit into from

Conversation

pboettch
Copy link
Contributor

Adds the operator+() to the json_pointer-class.

Fixes some comment/documentations as well.


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.

Please don't

  • The C++11 support varies between different compilers and versions. Please note the list of supported compilers. Some compilers like GCC 4.7 (and earlier), Clang 3.3 (and earlier), or Microsoft Visual Studio 13.0 and earlier are known not to work due to missing or incomplete C++11 support. Please refrain from proposing changes that work around these compiler's limitations with #ifdefs or other means.
  • Specifically, I am aware of compilation problems with Microsoft Visual Studio (there even is an issue label for these kind of bugs). I understand that even in 2016, complete C++11 support isn't there yet. But please also understand that I do not want to drop features or uglify the code just to make Microsoft's sub-standard compiler happy. The past has shown that there are ways to express the functionality such that the code compiles with the most recent MSVC - unfortunately, this is not the main objective of the project.
  • Please refrain from proposing changes that would break JSON conformance. If you propose a conformant extension of JSON to be supported by the library, please motivate this extension.
  • Please do not open pull requests that address multiple issues.

@FrancoisChabot
Copy link
Contributor

Wouldn't it be preferable to match std::filesystem::path and use operator/() instead?

@pboettch
Copy link
Contributor Author

Good point, at first I was convinced you're right, but reading the documentation std::filesystem::operator/() makes me now think otherwise:

std::filesystem::path does not have a operator/(). It is a global function in the std::filesystem-namespace which is used to

Concatenate(s) two path components using the preferred directory separator if appropriate (see operator/= for details).

The operator I'm adding is not doing that but it only adds 1 new token add the end of the pointer.

My C++-powers are not going far enough to see whether this can be easily achieved with such an operator as well (and keeping it simple). But I'm eager to learn.

@FrancoisChabot
Copy link
Contributor

FrancoisChabot commented Jan 24, 2019

As far as a member operator vs a namespace operator, the distinction is fairly light, beyond the academic merits of preferring free-floating functions in general.

A concrete advantage of using a namespace-level operator instead of a class member is that it allows to compose in the other direction as well. So you could eventually support: "abc" + pointer.. Arithmetic operators should always work in both directions after all, even if the result is not commutatively consistent.

This brings us to a more concrete reason to use operator/(). Since json_pointer is implicitly castable to std::string, with your current implementation, std::string + json_pointer works, and yields a concatenated string. The idea that reversing the order of operands to a + operation yields two entirely different types is pretty cringe-worthy to me.

On top of that operator/() makes more sense from a semantic standpoint. For me, the key distinction between + and / is that + is expected to be a dumb concatenation (like std::string), whereas / represents a "smart" concatenation (in that the path separator is implicit). In that sense, what you are doing is more akin to the later.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a06e7f5 on pboettch:develop into e89c946 on nlohmann:develop.

@garethsb
Copy link
Contributor

garethsb commented Jan 29, 2019

In my opinion, it does make sense to provide this feature via op/ and op/= as @FrancoisChabot suggested, like so:

class json_pointer
{
    // existing class members
    explicit json_pointer(const std::string& s = "")
    // ...
    std::string pop_back()
    // ...
    void push_back(const std::string& tok)
    // ...

    // concatenate two `json_pointer`s
    json_pointer& operator/=(const json_pointer& ptr); // effectively returns reference_tokens.insert(reference_tokens.end(), ptr.reference_tokens.begin(), ptr.reference_tokens.end()), *this
    friend json_pointer operator/(const json_pointer& lhs, const json_pointer& rhs); // effectively returns json_pointer(lhs) /= rhs

    // append a single unescaped `reference-token` to a `json_pointer`
    json_pointer& operator/=(std::string tok); // effectively returns this->push_back(std::move(tok)), *this
    json_pointer& operator/=(std::size_t index); // effectively returns *this /= std::to_string(index)
    friend json_pointer operator/(const json_pointer& lhs, std::string tok); // effectively returns json_pointer(lhs) /= std::move(tok)
    friend json_pointer operator/(const json_pointer& lhs, std::size_t index); // effectively returns json_pointer(lhs) /= index
};

The operation of removing the last reference-token is less common and back and pop_back serve this purpose, though adding a parent_pointer member function that returned a new json_pointer would mirror std::filesystem::path::parent_path nicely.

@pboettch
Copy link
Contributor Author

Thanks for suggesting this. @FrancoisChabot already convinced. A lack of time made me not getting forward. @garethsb-sony Feel free to get along with this PR with your suggestion.

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.

None yet

4 participants