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

Provide a from_json overload for std::map #1089

Merged
merged 1 commit into from
May 28, 2018

Conversation

theodelrieu
Copy link
Contributor

@theodelrieu theodelrieu commented May 14, 2018

This overload is chosen only when BasicJsonType::string_t
is not constructible from std::map::key_type.

Currently, converting a map to json treats it as an array of pairs.

fixes #1079


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.

@theodelrieu
Copy link
Contributor Author

@nlohmann Since we don't check extra values in the array when converting to std::pair, I'm doing the same thing.

(i.e. [1,2,3,4] -> std::pair {1, 2})

I'm wondering if we should check if the insertion in the map was successful, seems like a good idea. I don't know which exception to throw however. What's your opinion on it?

@coveralls
Copy link

coveralls commented May 14, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling c5e63fd on theodelrieu:feature/map_conversion into db03d09 on nlohmann:develop.

@theodelrieu theodelrieu force-pushed the feature/map_conversion branch from 6b6e73d to 0b94826 Compare May 14, 2018 12:07
@theodelrieu theodelrieu force-pushed the feature/map_conversion branch from 0b94826 to d44a273 Compare May 22, 2018 08:37
@nlohmann
Copy link
Owner

Sorry, I haven't found the time to look into this earlier.

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.

The rest of the PR is OK, just one little test case would be nice.

CHECK(m == m2);

json j7 = {0, 1, 2, 3};
CHECK_THROWS_WITH((j7.get<std::map<int, int>>()), "[json.exception.type_error.302] type must be array, but is number");
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please also add a CHECK_THROWS_AS test?

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

@nlohmann
Copy link
Owner

I think the behavior is OK as you're right: it is consistent to the std::pair treatment. Adding an exception for std::map would also require adding one for std::pair which would mean a breaking change in terms of semantic versioning, right?

@theodelrieu theodelrieu force-pushed the feature/map_conversion branch from d44a273 to 30d4789 Compare May 28, 2018 08:57
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.

@theodelrieu
Copy link
Contributor Author

theodelrieu commented May 28, 2018

Nothing is breaking, unless I missed something.

The std::pair overload is already there, and does not need to be changed. The issue with std::map is that its value_type is std::pair<**const** Key, Value>, so using the default std::pair overload does not work.

The only way that I know of constructing this peculiar value_type is to use std::map constructors/functions, so this special treatment is done inside the std::map overload.

Also, one can already serialize a std::map<int, int> to json, the other way around does not work though.

@nlohmann
Copy link
Owner

With "breaking" I meant the API behavior in case of superfluous list entries. As we do not throw for pairs if we convert from a 3-element list, we should not throw for maps for symmetry reasons. But adding an exception to the pairs would be a breaking change.

This overload is chosen only when BasicJsonType::string_t
is not constructible from std::map::key_type.

Currently, converting a map to json treats it as an array of pairs.

fixes nlohmann#1079
@theodelrieu theodelrieu force-pushed the feature/map_conversion branch from 30d4789 to c5e63fd Compare May 28, 2018 09:06
@theodelrieu
Copy link
Contributor Author

Oh you're right, I've added a test case for this, I kept the existing behavior (i.e. discarding extra values from arrays)

@nlohmann nlohmann self-assigned this May 28, 2018
@nlohmann nlohmann added this to the Release 3.1.3 milestone May 28, 2018
@nlohmann
Copy link
Owner

Just for my understanding: Assume I have MyType and provided a to_json function. Then std::map<int, MyType> will be serialized to a list of pairs, and std::map<std::string, MyType> will be serialized to an object.

Assume I have AnotherType that can be converted to a std::string, e.g. by operator std::string(). Will a std::map<AnotherType, MyType be serialized to an object?

@theodelrieu
Copy link
Contributor Author

Indeed, it will be serialized to an object.

The condition is: if string_t is constructible from Key, then it will be serialized to an object.

@nlohmann nlohmann merged commit 0efaf89 into nlohmann:develop May 28, 2018
@nlohmann
Copy link
Owner

Thanks a lot!

@theodelrieu theodelrieu deleted the feature/map_conversion branch May 28, 2018 14:34
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.

Compilation error with strong typed enums in map in combination with namespaces
3 participants