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

Add a get overload taking a parameter. #1231

Merged
merged 1 commit into from Sep 29, 2018

Conversation

theodelrieu
Copy link
Contributor

@theodelrieu theodelrieu commented Sep 10, 2018

Closes #1227.

Takes an lvalue reference, and returns the same reference.

This allows non-default constructible types to be used with get (i.e. without specializing adl_serializer).
This overload does not require CopyConstructible either.

// New value use-case
auto values = j.get<std::vector<std::string>>();

// already created values
std::string str;
j.get(str);

Implements #1227

This can be a step forward to resolve #958. Indeed, we can recommend users to replace implicit conversions from json values by using this new overload. I believe we could start deprecating operator T if we decide to merge this PR.

Note that this new overload returns its parameter, which allows chaining (i.e. a bit like C++17's vector::emplace_back).


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.

@coveralls
Copy link

coveralls commented Sep 10, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 521fe49 on theodelrieu:feature/get_with_parameter into 680a4ab on nlohmann:develop.

This overloads is chosen if:
- @a ValueType is not @ref basic_json,
- @ref json_serializer<ValueType> has a `from_json()` method of the form
`void from_json(const basic_json&, ValueType&)`, and
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment seems to end in the middle of the sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, will fix

@theodelrieu
Copy link
Contributor Author

Should I add support for basic_json instances as well? It's supported for the first get overload.

@nlohmann
Copy link
Owner

What do you mean?

@theodelrieu
Copy link
Contributor Author

// one can do the following
nlohmann::json j;
auto v = j.get<my_type>();
auto j2 = j.get<nlohmann::json>();
auto j3 = j.get<my_custom_json>();

Should we support the following:

nlohmann::json j;
nlohmann::json j2;
my_custom_json j3;
j.get(j2);
j.get(j3);

It would be consistent with the non-parameter overload.

@theodelrieu
Copy link
Contributor Author

I've added support for basic_json types.

I also added tests in unit-conversions.cpp, note that converting to C-arrays is possible with this overload.

However, it will throw when using string_t::value_type []. It's the current behavior with std::array<string_t::value_type, N>. It would be nice to fix both at the same time.

But this is a bit tricky, it requires retrieving the current string, and throw(?) if the string length does not match the C-array/std::array size...

@theodelrieu theodelrieu force-pushed the feature/get_with_parameter branch 3 times, most recently from 7384a9a to db5e00a Compare September 13, 2018 08:33
@theodelrieu
Copy link
Contributor Author

@nlohmann I think it would be good to release a 3.2.1 with #1238 in it before merging this PR. What do you think?

@nlohmann
Copy link
Owner

@theodelrieu Why not both?

@theodelrieu
Copy link
Contributor Author

theodelrieu commented Sep 14, 2018

I don't really mind, but since it's a new feature it would be more fit in a minor release.

We can make 3.3 directly though

@nlohmann
Copy link
Owner

Oh, I see. Maybe I need to better understand the feature. Could you add a snippet for the README?

@theodelrieu
Copy link
Contributor Author

I knew I forgot something...

@theodelrieu theodelrieu mentioned this pull request Sep 17, 2018
4 tasks
README.md Outdated
@@ -226,6 +226,9 @@ json j_string = "this is a string";
std::string cpp_string = j_string;
// retrieve the string value (explicit JSON to std::string conversion)
auto cpp_string2 = j_string.get<std::string>();
// retrieve the string value (alternative explicit JSON to std::string conversion)
std::string cpp_string3;
j_string.get(cpp_string3);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be so late to the party, but now when I see this in the documentation: isn't the name get a bit misleading? It's rather a setter, but to the passed reference. But in particular the example below

j.at("name").get(p.name);

seems counter-intuitive to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe get_to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_to sounds good to me.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, could you please change the name to get_to and also add an example to doc/examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Takes an lvalue reference, and returns the same reference.

This allows non-default constructible types to be converted without
specializing adl_serializer.
This overload does not require CopyConstructible either.

Implements nlohmann#1227
@theodelrieu
Copy link
Contributor Author

Let me know if the example looks fine, I could not upload them to wandbox by running make -C doc

@nlohmann nlohmann self-assigned this Sep 29, 2018
@nlohmann nlohmann added this to the Release 3.2.1 milestone Sep 29, 2018
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann merged commit d26f394 into nlohmann:develop Sep 29, 2018
@nlohmann
Copy link
Owner

Thanks a lot! Great work as always.

@theodelrieu theodelrieu deleted the feature/get_with_parameter branch October 1, 2018 13:55
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.

json::get function with argument operator T() considered harmful
4 participants