-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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 minimal annotation support #4082
Closed
marekpiotrowski
wants to merge
13
commits into
nlohmann:develop
from
marekpiotrowski:add-minimal-annotation-support
Closed
Add minimal annotation support #4082
marekpiotrowski
wants to merge
13
commits into
nlohmann:develop
from
marekpiotrowski:add-minimal-annotation-support
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @marekpiotrowski |
🔴 Amalgamation check failed! 🔴The source code has not been amalgamated. @marekpiotrowski |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Hey @nlohmann !
Hope you're having a great day!
After reading the guidelines and your discussion around comments in JSON, I got a feeling that this PR has no chance at all :D So, I was about to start writing documentation and considering I'd like to avoid redundant work, I though it'd make sense to check with you first.
Long story short, it's about annotating custom classes (annotations known at compile time) and dumping those when serializing (please see the tests or the example):
would produce:
If you'd ever consider merging this PR and allowing attachment of annotations known at compile time, I'll make sure that the documentation is consistent, code is amalgamated, will check the coverage, make sure redundant files are cleaned up etc. as per the guidelines. However, if you think it's a bad idea to introduce such functionality, I'm happy to keep this as a fork. I'll occasionally merge to your latest changes I guess.
The reason is that we've got plenty of JSON configuration files which are supposed to be filled by non-technical people. We use your brilliant plugin cus it's very robust and allows for ignoring comments when parsing. We've been annotating the dumped JSONs with some hard-core text manipulations in "post-processing", but it's horrible. Maintaining such documentation outside of the code is pretty much impossible so we had no choice though.
But now I thought that I could slightly extend your serializer and convenient macros for annotating the properties of custom classes.
Please let me know if there's any chance we could merge such functionality. If that's not the case, I'll have to prepare the documentation differently ;)
Cheers,
Marek