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

conversion from/to user-defined types #435

Merged
merged 101 commits into from Jan 27, 2017
Merged

Conversation

nlohmann
Copy link
Owner

This PR is #423 after a small cleanup. Nothing substantial has changed - mostly documentation was touched.

Théo Delrieu and others added 30 commits January 21, 2017 16:14
the first is the basic_json type, the second the user-defined type
Copy link
Contributor

@theodelrieu theodelrieu left a comment

Choose a reason for hiding this comment

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

I added a few comments (mainly those you added to the previous PR)

// I'm sure there is a better way, but this might need a big basic_json refactoring
template <value_t> struct external_constructor;
// I'm sure there is a better way, but this might need a big basic_json
// refactoring
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove a part of this comment, we could only keep the part about the 'there might be a better way'

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ok.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I removed the comment altogether.

```

It works, but that's quite a lot of boilerplate.. Hopefully, there's a better way:
It works, but that's quite a lot of boilerplate... Hopefully, there's a better way:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you talked about replacing 'Hopefully' with 'Fortunately'

Copy link
Owner Author

Choose a reason for hiding this comment

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

I forgot about that, thanks!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Changed.

} // namespace ns
```

That's all. When calling the json constructor with your type, your custom `to_json` method will be automatically called.
That's all! When calling the `json` constructor with your type, your custom `to_json` method will be automatically called.
Likewise, when calling `get<your_type>()`, the `from_json` method will be called.

Some important things:

* Those methods **MUST** be in your type's namespace, or the library will not be able to locate them (in this example, they are in namespace `ns`, where `person` is defined).
Copy link
Contributor

Choose a reason for hiding this comment

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

About that part, I don't know if we should add a part mentioning that it can work in the global namespace.
It might not be clear for everyone.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think about how to formulate this briefly.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I added a small comment.

Likewise, when calling `get<your_type>()`, the `from_json` method will be called.

Some important things:

* Those methods **MUST** be in your type's namespace, or the library will not be able to locate them (in this example, they are in namespace `ns`, where `person` is defined).
* When using `get<your_type>()`, `your_type` **MUST** be DefaultConstructible and CopyConstructible (There is a way to bypass those requirements described later)
* When using `get<your_type>()`, `your_type` **MUST** be [DefaultConstructible](http://en.cppreference.com/w/cpp/concept/DefaultConstructible) and [CopyConstructible](http://en.cppreference.com/w/cpp/concept/CopyConstructible). (There is a way to bypass those requirements described later.)
Copy link
Contributor

Choose a reason for hiding this comment

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

I removed the need for CopyConstructible. This way, some types can be used thanks to copy elision.
If you think this is not a really good idea, you could simply put that check back in the static_assert

Copy link
Owner Author

Choose a reason for hiding this comment

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

I removed mention of CopyConstructible.

@nlohmann
Copy link
Owner Author

Thanks for the comments! I shall have a look later this week.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4139bb6 on theodelrieu-feature/fromtojson into ce0b3fe on develop.


#### Basic usage

To make this work with one of your types, you only need to provide two methods:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe: "... two functions" instead of "... two methods". Method sounds like a class method, whereas here it really is two free-standing functions.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Good point!

@theodelrieu
Copy link
Contributor

theodelrieu commented Jan 25, 2017

We could also add a sentence about enums. Right now, only enum class are serializable via from_json/to_json without specializing adl_serializer

EDIT: I'm wrong, all wrong, everything works

@@ -109,7 +109,7 @@ RECURSIVE = NO
EXCLUDE =
EXCLUDE_SYMLINKS = NO
EXCLUDE_PATTERNS =
EXCLUDE_SYMBOLS = nlohmann::anonymous_namespace
EXCLUDE_SYMBOLS = nlohmann::detail
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we bump the version number here too?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I shall change the version number once I created a release branch.

@nlohmann
Copy link
Owner Author

@theodelrieu I would not know what to write about the enums - don't they work the same?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ec03c9c on theodelrieu-feature/fromtojson into ce0b3fe on develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4d3053c on theodelrieu-feature/fromtojson into ce0b3fe on develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4d3053c on theodelrieu-feature/fromtojson into ce0b3fe on develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 77bb7af on theodelrieu-feature/fromtojson into ce0b3fe on develop.

@theodelrieu
Copy link
Contributor

Well, I thought they didn't, since I added this overload in detail:

template<typename BasicJsonType, typename UnscopedEnumType,
         enable_if_t<is_unscoped_enum<UnscopedEnumType>::value, int> = 0>
void to_json(BasicJsonType& j, UnscopedEnumType e) noexcept { /* ... */ }

However I tested it with the following code, and it does work:

enum test{};
void to_json(json& j, test t) {} // this method is called

I thought this would be an ambiguous call, but since the second one is a perfect match, it isn't!

So, nevermind what I said, no need to add it in the doc!

// will not be able to convert move_only_type to json, since you fully
// specialized adl_serializer on that type
static void to_json(json& j, move_only_type t) {
j = t.i;
Copy link
Contributor

Choose a reason for hiding this comment

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

t.i is private here

Copy link
Owner Author

Choose a reason for hiding this comment

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

Right... Did I change this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, but just removing private: is fine for this example

Some important things:

* Those methods **MUST** be in your type's namespace (which can be the global namespace), or the library will not be able to locate them (in this example, they are in namespace `ns`, where `person` is defined).
* When using `get<your_type>()`, `your_type` **MUST** be [DefaultConstructible](http://en.cppreference.com/w/cpp/concept/DefaultConstructible). (There is a way to bypass those requirements described later.)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rephrase the sentence in parenthesis : (There is a way to bypass this requirement described later).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling cd9701b on theodelrieu-feature/fromtojson into ce0b3fe on develop.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling cd9701b on theodelrieu-feature/fromtojson into ce0b3fe on develop.

@nlohmann nlohmann merged commit 42fa3f0 into develop Jan 27, 2017
@nlohmann nlohmann deleted the theodelrieu-feature/fromtojson branch January 27, 2017 05:22
@nlohmann
Copy link
Owner Author

Thanks for the feedback, @jaredgrubb !

And thanks for the awesome implementation @theodelrieu !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants