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 public push_back, pop_back #837

Closed
HugoLopata opened this issue Nov 20, 2017 · 20 comments
Closed

json_pointer public push_back, pop_back #837

HugoLopata opened this issue Nov 20, 2017 · 20 comments
Labels
kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@HugoLopata
Copy link

Feature Request
I am generating some json pointers. It would be handy using push_back instead of composing string with slashes, which is subsequently broken to vector of strings by json_pointer.

Access to internal data may be handy too:
const vector<string>& json_pointer::impl() const;

@nlohmann
Copy link
Owner

I think this addition makes sense - especially as reference tokens are stored inside a vector internally.

@nlohmann nlohmann self-assigned this Nov 25, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone Nov 25, 2017
nlohmann added a commit that referenced this issue Nov 25, 2017
- made pop_back public and added documentation
- implemented push_back function and added documentation
- added test cases
@nlohmann
Copy link
Owner

@HugoLopata I added the functions to a feature branch, see 516d4e6. Is this what you had in mind?

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Nov 25, 2017
@HugoLopata
Copy link
Author

HugoLopata commented Nov 27, 2017

Yes, exactly as expected.
Please add note to the documentation about difference between constructor and push_back.
Push_back do not unescape and slash is simply slash. See an example.

json::json_pointer ptr1("/foo/bar~0baz");
json::json_pointer ptr2;
ptr2.push_back("/foo/bar~0baz");
std::cout << "ptr1: " << ptr1.to_string() << '\n'; // /foo/bar~0baz
std::cout << "ptr2: " << ptr2.to_string() << '\n'; // /~1foo~1bar~00baz

Consider also adding some examples with escape characters to the tests.

@nlohmann
Copy link
Owner

Oh I did not think about escaping. What do you think? Should push_back escape slashes?

@HugoLopata
Copy link
Author

  • Variant 1: Single level, unescaped - as proposed in patch. This behavior is little bit inconsistent. Push_back is single level operation. No escaping needed. -> Simple usage, i like it.
  • Variant 2: Single level, escaped - if you use the same algorithm as in constructor, then slashes should be denied (user should use ~0 instead). Consistent with constructor, but user has to do escaping (some public static string escape(const &string) will be convenient then).
  • Variant 3: Multilevel push_back. I don't like it.

@theodelrieu
Copy link
Contributor

I am not fond of push_back, it's way too associated with containers.

However I think adding both operator/ and operator/= would be great.

Whether these escape or not should depend on the most intuitive/expected outcome. I've never used JSON pointers, so this should be agreed upon by experienced users.

Anyway we have to support both cases, so mimicking std::filesystem::path seems reasonable (i.e. having operator+= and concat)

Boost.Filesystem decided to not have an operator+, I haven't seen the exact rationale, but my guess would be that they feared a lot of misuses would have happened with it.

I believe json_pointer is very similar to std:: filesystem::path, so having a similar interface makes sense to me.

@nlohmann
Copy link
Owner

nlohmann commented Dec 7, 2017

I agree that push_back is indeed associated with containers, and I think the idea of operator/ and operator/= is charming.

@nlohmann nlohmann removed this from the Release 3.0.0 milestone Dec 16, 2017
@nlohmann
Copy link
Owner

I will postpone a decision for this after the 3.0.0 release.

@stale
Copy link

stale bot commented Jan 15, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jan 15, 2018
@nlohmann nlohmann removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jan 17, 2018
@stale
Copy link

stale bot commented Feb 16, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Feb 16, 2018
@HugoLopata
Copy link
Author

ping
i can't see any alternative of pop_back in solution with operator/.
Moreover adding something with operator/ is little bit unexpected. Operator+ is more convenient even though the separator is a slash. And there can be operator-- (maybe both pre and post-decrement) as alternative to pop_back.

@stale stale bot removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Feb 22, 2018
@theodelrieu
Copy link
Contributor

theodelrieu commented Feb 22, 2018

I don't think there should be an alternative to operator/.

Just like std::filesystem::path has remove_filename, having a function with an explicit name to do such an operation seems like a good idea to me.

Concatenation is one of the most usual things to do with path-like objects, whereas removing a specific part is not done that often.

Moreover, overloading operator-- on a class that is not a number or an iterator would be very surprising.

@nlohmann
Copy link
Owner

We seem to circle around naming things...

  • I like push_back/pop_back for their symmetry, but can understand the association to containers which may be confusing.
  • I like operator/ and operator/= for the added syntactic sugar, but putting a remove_token (or whatever pop_back alias) next to it, feels clumsy. operator-- would be the worst from all worlds...

Any more ideas?

Furthermore, #837 (comment) needs to be addressed.

@nlohmann nlohmann added state: please discuss please discuss the issue or vote for your favorite option and removed solution: proposed fix a fix for the issue has been proposed and waits for confirmation labels Feb 25, 2018
@nlohmann nlohmann removed their assignment Feb 25, 2018
@theodelrieu
Copy link
Contributor

I would go for variant 1 too, since push_back adds a single path component to the current object, it seems reasonable to me that the argument should be escaped.

@stale
Copy link

stale bot commented Mar 28, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Mar 28, 2018
@stale stale bot closed this as completed Apr 4, 2018
@nlohmann nlohmann removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Apr 17, 2018
@nlohmann nlohmann reopened this Apr 17, 2018
@SylvainCorlay
Copy link

SylvainCorlay commented Apr 17, 2018

I don't think that using operator/= would be a good idea: even though it would seem clever because the separator in the JSON spec is /, this would prevent using STL generic algorithms on json_pointer. Instead, a better API would be that of a generic STL container of strings, which would also follow the principle of least surprise.

Adding a few item to a jsoon_pointer would be

jp.insert(jp.end(), {
    "few",
    "more",
    "items"
});

instead of jp.push_back("few/more/items").

A good way to achieve this without JSON pointers to match std::vector<std::string> in signatures would be to use private inheritance and add a few using statement such as

  • using base_type::base_type; for all the constructor
  • using base_type::begin/end/insert/push_back etc for all methods.

Nothing would prevent additional constructors to be added for slash-separated strings, but I think that the user-defined literal is probably enough already for that.

@theodelrieu
Copy link
Contributor

this would prevent using STL generic algorithms on json_pointer

Please note that having both an operator and a member function is not incompatible, there is std::fs::path::operator/= and std::fs::path::append which has the exact same behavior.

In this case, generic code could use insert/append, while users of json::pointer in non-generic code can choose to use the operator/= for convenience.

@stale
Copy link

stale bot commented May 30, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label May 30, 2018
@stale stale bot closed this as completed Jun 6, 2018
@epvbergen
Copy link

I also need to split and concatenate and iterate over json_pointer elements. I think one could sidestep the escaping issue by only exposing json_pointers on the interface for that, not the underlying std::string. Where the conversion happens is then an implementation detail exposed only elsewhere.

However, exposing the semantics of the underlying std::vector feels fine. If a json_pointer could walk and quack like a std::vector<json_pointer> with automatic flattening, things would be perfect.

@pboettch
Copy link
Contributor

Thanks Niels for referring me to this issue. My PR is extremely simple and works.

I have an itch right now, that I have to scratch. In my validator library I'm doing horrible things to append a key to a nlohmann::json_pointer. (https://github.com/pboettch/json-schema-validator/blob/master/src/json-schema.hpp#L95)

Here my suggestions: make pop_back() private again and add a append(), remove() method. Syntactic sugar can be added later. Methods working with iterators as well if need.

Of course it would be nice to have similar interface as a std::filesystem::path. Do we need all of it at once or can it be done with iterations? Starting by adding append() and remove_end()/remote_back() would be good start, wouldn't it? ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

No branches or pull requests

6 participants