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

Introduce try_get() and try_deserialize() #2288

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

oficsu
Copy link

@oficsu oficsu commented Jul 18, 2020

I propose new method json.try_get<MyType>(), which works similar to json.get<MyType>(). It can be extended by providing adl_serializer<MyType>::try_deserialize() function. Unlike get method, the return type of try_get is assumed to have a different return type to MyType and can be std::optional, tl::expected or other wrapper

Why is it needed?

  1. Users may prefer to handle errors without exceptions for the sake of performance
  2. Or, some users like more functional program structuring, for example, using monads, this allows not only to handle errors, but also to return some additional info about deserialization, such as warnings or even detailed debug logs.
  3. Maybe something else?

Why not just allow json.get<MyType>() to return any type, different from MyType?

  1. This is not obvious code behavior and some users may not expect a different return type
  2. Some users may want to explicitly distinguish between json.get<MyType>() and json.try_get<MyType>(), or even have both

What about json.get<MyMonad<MyType>>()? Why not?

  • Users can define their own version of try_get() with custom return type once and don't bother about return type anymore every time get() is called. This is quite important in a large codebase where get() calls many times for same type
  • Or, even more importantly, I think json.try_get<MyType>() is easier to read and is a good abstraction compared to json.get<MyMonad<MyType>>()
  • Also, I think json.try_get<T>() is the better way to return optional than json.get<std::optional<T>>() from Fix conversion from empty json to std::optional<> #2229

So, is noexcept our goal? Does json.try_get<X>() throw exceptions?

  • It's depend on user implementation of try_deserialize() and it is the responsibility of the users to mark try_deserialize() as noexcept(true) or noexcept(false).

Should we implement try_deserialize() for basic types?

  • I'm not sure. tl::expected or std::optional could be suitable for this, but most likely, the user will want to write their own monadic type or wrapper.
    Also, there may be a problem when users include two libraries that use nlohman_json and specializing different adl_serializer for the same basic type. Maybe we can implement try_deserialize lookup through tag dispatching with adl and allow additional arguments to be passed to try_get(), then users can create their own unambiguous overloads. More research is needed here and right now I think the answer is no

About naming

  • I'm not sure about the naming. I think, try_deserialize() is too contrast to from_json(), but I don't know what name is better

@oficsu oficsu requested a review from nlohmann as a code owner July 18, 2020 16:55
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7f4d035 on oficsu:try_deserialize_squash into 43ab8a2 on nlohmann:develop.

@oficsu oficsu changed the title introduce try_get() and try_deserialize() Introduce try_get() and try_deserialize() Jul 18, 2020
@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Jul 19, 2020
@oficsu oficsu marked this pull request as draft July 20, 2020 21:14
@stale
Copy link

stale bot commented Aug 29, 2020

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 Aug 29, 2020
@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 Aug 30, 2020
@oficsu
Copy link
Author

oficsu commented Sep 10, 2020

After thinking about this PR again, I decided that it requires some more work. Perhaps design could be better. I don't have much free time at the moment, so I'll come back later and think about this PR again. Do I need to close PR for this time or can I leave PR as a draft?

@stale
Copy link

stale bot commented Oct 12, 2020

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 Oct 12, 2020
@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 Oct 19, 2020
@SpiritDogstar
Copy link

In addition, it would be handy to have the macros such as: NLOHMANN_DEFINE_TYPE_INTRUSIVE use a version which is ok if the data isn't found. try/catching everything can be expensive if "not found" is an acceptable state.

Expanding even further to the "optional is ok", bool from_json(.......) whereby validation can be reported as a warning, or diagnostic, but not as an error that is try/catch.

It means, NLOHMANN_DEFINE_TYPE_INTRUSIVE isn't as useful, or jsons become bigger with required params being null instead of leaving them out.

An example additional macro:
NLOHMANN_DEFINE_TYPE_TRY_INTRUSIVE(.....)
In this simple macro naming, it's "all or none", and that's ok as optional might be the common usecase.

Additionally, having from_json (and thus, macro) return true/false, or count of missing params, or a tuple of found / maxcount would be helpful and allow for custom detection.

@stale
Copy link

stale bot commented Dec 25, 2020

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 Dec 25, 2020
@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 10, 2021
@axic
Copy link
Contributor

axic commented Nov 7, 2021

Would it make sense extending this to have an optional default value?

I am in the process of porting over a project from jsoncpp to nlohmann-json, and the former has this as a very convenient feature (for us at least), i.e. .get("field", Json::object()) to get an optional empty object if not present.

Is there a best practice for this in the current library?

@nlohmann
Copy link
Owner

nlohmann commented Nov 7, 2021

Would it make sense extending this to have an optional default value?

I am in the process of porting over a project from jsoncpp to nlohmann-json, and the former has this as a very convenient feature (for us at least), i.e. .get("field", Json::object()) to get an optional empty object if not present.

Is there a best practice for this in the current library?

The value function should do this: https://json.nlohmann.me/api/basic_json/value/.

@axic
Copy link
Contributor

axic commented Nov 7, 2021

@nlohmann oh wow, that certainly looks like it, thank you!

@stale
Copy link

stale bot commented Jan 9, 2022

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 9, 2022
@gregmarr
Copy link
Contributor

After thinking about this PR again, I decided that it requires some more work. Perhaps design could be better. I don't have much free time at the moment, so I'll come back later and think about this PR again. Do I need to close PR for this time or can I leave PR as a draft?

@oficsu Any progress here?

@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 Jun 18, 2022
@oficsu
Copy link
Author

oficsu commented Jun 19, 2022

@gregmarr, no, I couldn't come up with a better design

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: please discuss please discuss the issue or vote for your favorite option
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants