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

Can we have an exists() method? #1471

Closed
p-i- opened this issue Feb 1, 2019 · 13 comments
Closed

Can we have an exists() method? #1471

p-i- opened this issue Feb 1, 2019 · 13 comments
Assignees
Labels
release item: ✨ new feature solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@p-i-
Copy link

p-i- commented Feb 1, 2019

I wish to determine whether a particular key exists.

This is one of those things it's reasonable to expect, and surprising that there is no easy / obvious / direct solution.

is_null() (rather dangerously IMHO) silently creates the key.

https://stackoverflow.com/questions/49850890/c-nlohmann-json-how-to-test-nested-object-is-existent-or-empty suggests .count() but again is not obvious how to apply this.

I've seen other solutions suggested involving iterators.

Pretty sure I saw a nice solution the other day in an issue, but now I can't find it.

Why not simply have an exists() method?

The documentation is super helpful for ENcoding JSON, but lacking in code examples for extracting from JSON.

@nlohmann
Copy link
Owner

nlohmann commented Feb 1, 2019

You could implement this yourself with

bool exists(const json& j, const std::string& key)
{
    return j.find(key) != j.end();
}

@gregmarr
Copy link
Contributor

gregmarr commented Feb 1, 2019

is_null() (rather dangerously IMHO) silently creates the key.

No it does not. There is no key involved in an is_null() call, so it can't create a key.

If you are referring to your j["bar"].is_null() test, then it is j["bar"] that creates the key, just like in std::map. This is required in order to enable things like j["bar"] = 1;.

You will get the same result on the last line if you leave out the .is_null().

json j = {
    { "foo", 1 }
};

std::cout << j["bar"] << std::endl;

std::cout << j << std::endl;
// {"bar":null,"foo":1}

@gregmarr
Copy link
Contributor

gregmarr commented Feb 1, 2019

The answer to your question is the same as the answer to this question: https://stackoverflow.com/questions/1939953/how-to-find-if-a-given-key-exists-in-a-c-stdmap

@ammaratef45
Copy link

You could implement this yourself with

bool exists(const json& j, const std::string& key)
{
    return j.find(key) != j.end();
}

@nlohmann I think it's better to be available too.
It's the way we think of JSON using (we search for the key and act differently when it's found or not found)
If everyone would implement that so you should add it (I guess)

@hydratim
Copy link

hydratim commented Feb 5, 2019

Make a PR.
Why put all the pressure on the maintainer? It's an easy feature for anyone to contribute.

@nlohmann
Copy link
Owner

nlohmann commented Feb 5, 2019

I'm still torn to extend the API, because the same can be achieved with either find (see above) or count (as mentioned in the original post). I'm afraid we blow the API, and since "exists" is also nothing someone would "know" from an STL container, it's not necessarily making the API more intuitive.

So let's discuss!

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Feb 5, 2019
@garethsb
Copy link
Contributor

garethsb commented Feb 5, 2019

I guess that exists or something like it is expected by people who aren't thinking of json like a std container. If you're a regular user of find and count, etc. on a std::map, the current API makes great sense.

Interestingly I see cppreference describes contains as coming in C++20... https://en.cppreference.com/w/cpp/container/map/contains

@ammaratef45
Copy link

exists or has makes the readability much better

I want people who read my code not to feel like they are reading an STD c++ code but an Object-Oriented code

However, contains looks pretty nice to use too.

@ammaratef45
Copy link

ammaratef45 commented Feb 6, 2019

Make a PR.
Why put all the pressure on the maintainer? It's an easy feature for anyone to contribute.

@hydratim
I agree with you, but If the maintainer refuses it from the beginning why should I waste time? the discussion goes first then coding, right?

@nickaein
Copy link
Contributor

nickaein commented Feb 6, 2019

I'm afraid we blow the API, and since "exists" is also nothing someone would "know" from an STL container, it's not necessarily making the API more intuitive.

I strongly agree. If we start to diverge from STL containers API, there are many possibilities and choices to the API design and a seasoned C++ developer have to put effort to learn this custom API. Even a little divergence can be confusing and irritating since the user cannot trust any longer that our API has the same behavior and semantics of the STL. Having same API as the STL has this benefit that a user familiar with STL can intuitively work with this library too. It is reasonable to expected the user of this library is familiar with STL containers and their API.

Interestingly I see cppreference describes contains as coming in C++20.

Thanks @garethsb-sony! This looks promising. The original paper makes some good arguments too, specifically on the member-vs-free function.

This proposal has been merged into the latest C++2a draft (N4800) (Table 69 which specifies Associative container requirements). Also, Boost has already adopted it. So even though C++20 is not finalized yet, this addition seems to be a done deal.

Based on these, I believe adding contains() member function is the way to go for such feature which can be helpful for users while we keep the compatibility with the latest C++ STL standard.

@garethsb
Copy link
Contributor

garethsb commented Feb 6, 2019

If we start to diverge from STL containers API, there are many possibilities and choices to the API design and a seasoned C++ developer have to put effort to learn this custom API. Even a little divergence can be confusing and irritating since the user cannot trust any longer that our API has the same behavior and semantics of the STL.

Agreed (and as a little aside, is the reason that while preparing #1469, I was a little surprised to find that json_pointer::pop_back returns a value).

@nickaein
Copy link
Contributor

I've implemented in PR #1474. Feedback/more discussions are welcomed!

@nlohmann
Copy link
Owner

Closed by merging #1474.

@nlohmann nlohmann added solution: proposed fix a fix for the issue has been proposed and waits for confirmation release item: ✨ new feature and removed state: please discuss please discuss the issue or vote for your favorite option labels Feb 13, 2019
@nlohmann nlohmann self-assigned this Feb 13, 2019
@nlohmann nlohmann added this to the Release 3.5.1 milestone Feb 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release item: ✨ new feature solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

7 participants