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

No-throw json::value() #1733

Closed
PerMalmberg opened this issue Sep 1, 2019 · 7 comments
Closed

No-throw json::value() #1733

PerMalmberg opened this issue Sep 1, 2019 · 7 comments
Labels
kind: enhancement/improvement state: needs more info the author of the issue needs to provide more details

Comments

@PerMalmberg
Copy link

Currently, it's a must to write code like this to prevent exceptions in the case optional values are omitted from the underlying data:

auto id = cfg.contains("device_id") ? cfg.value("device_id", "") : "";

A more streamlined version would be like this, specifying no-throw, similar to json::parse:

auto id = cfg.value("device_id", "", false);

Allowing value() to take an allow_exception, same as json::parse would reduce boiler place code and also meet user expectations; we're providing a default value, but instead of returning that, value() throws an exception.

Thoughts?

@jaredgrubb
Copy link
Contributor

This already works, unless you're unsure if cfg is a dictionary. I don't think having .value return a default when cfg is actually a number or null is a good idea, personally.

FWIW, your code should be something like auto id = cfg.is_object() ? cfg.value("device_id", "") : "";, which will be far more efficient and have the same behavior.

@PerMalmberg
Copy link
Author

That is actually exactly the case.

When the JSON-file (i.e. the configuration) is empty, json::parse(.... false) allows to "load" an empty string, where upon I want to read default values for everything. Personally, I think this addition would bring a little symmetry to the API; if parse can load an empty string, shouldn't value then be able to return the default value from that same (empty) data?

@nlohmann
Copy link
Owner

nlohmann commented Sep 2, 2019

What would your code do if the proposed default value does not match the existing value:

json j;
j["device_id"] = 10;
auto id = j.contains("device_id") ? j.value("device_id", "") : "";

(This currently throws: [json.exception.type_error.302] type must be string, but is number).

@nlohmann nlohmann added the state: needs more info the author of the issue needs to provide more details label Sep 2, 2019
@PerMalmberg
Copy link
Author

I might be missing your point, but I'd consider that a programmer error; i.e. mixing up types. I'd expect it to return the provided default value as that is what I'm asking for.

Had I wanted an integer as a default value, then I'd call value with an int as a default value.

@nlohmann
Copy link
Owner

nlohmann commented Sep 2, 2019

The thing is: you do not know which value is actually stored in the object. You may have expected a string and provided "" as default if the key was not found. But when a number was stored instead, only an exception makes sense. What I mean: by defining away the situation where you call value() on a JSON value other than an object, you do not get rid of all the exceptions.

@PerMalmberg
Copy link
Author

You're right in that I don't know what value has actually been stored, but I'm 100% sure what the application expects, which is exactly why the ability to provide a default value without the possibility of an exceptions would be such a good thing.

If the application expects a string but a number has been stored, then there's a mismatch between expected and actual data. This could for example happen when a user manually edits a JSON configuration and enters { "key": "1" } instead of { "key": 1 } or I myself assign a variable to the wrong key at run time.

What is the correct behavior in this case? I'd say it is up to the application to decide; either 1) throw an exception (as it currently does), catch it and handle it by using a default value, or 2) catch the exception, log and exit.

In my case - an embedded system with no screen or keyboard - using default values is always the preferred option instead of entering a boot-loop due to bad configuration.

If this still doesn't makes sense to you, feel free to close this issue, I'll fallback to writing a wrapper for reading values.

@nlohmann
Copy link
Owner

nlohmann commented Sep 3, 2019

My point is that even your "no-throw" value() could still throw, and I agree with #1733 (comment) that in the end it would be clearer to write

auto id = cfg.is_object() ? cfg.value("device_id", "") : "";

as this makes it explicit that you are OK with cfg not being an object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement state: needs more info the author of the issue needs to provide more details
Projects
None yet
Development

No branches or pull requests

3 participants