Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 strict enum de/serialization macro #4612
base: develop
Are you sure you want to change the base?
add strict enum de/serialization macro #4612
Changes from 8 commits
06b667c
4ebebad
0083937
c6d9ea0
4444985
e7c021f
32cbaee
2898659
11165ef
0cda972
e25fc04
5f7d41c
6747555
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"If a string value" since it's not an arbitrary json value but a string....
Oh, OH, it doesn't require that it be a string, does it?
😮
There's nothing in the code that requires that this be a string.
@hnampally Is this intended behavior, or did this just accidentally fall out of the second parameter being
BasicJsonType
instead ofstd::string
?If this is intended behavior, it should definitely be documented before someone trips upon it accidentally, or someone relies on it and then someone else "fixes" it.
If it's not intended behavior, then it should be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gregmarr thanks for bringing this up but I am not really sure about the requirements for this feature but I am open for either, @nlohmann please weigh in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the empty line.
Also: move line
json j_yellow = "yellow";
above the comment
// deserialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the exception message. What about
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that sounds better, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: