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

Support for comments. #376

Closed
TurpentineDistillery opened this issue Dec 3, 2016 · 27 comments · Fixed by #2212
Closed

Support for comments. #376

TurpentineDistillery opened this issue Dec 3, 2016 · 27 comments · Fixed by #2212
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@TurpentineDistillery
Copy link

One of the biggest gripes people have with json format is the lack of support for comments.
E.g. see this reddit thread.

Many libraries, including jsoncpp and RapidJSON have support for comments in spite of the RFC.
Additionally, RapidJSON allows further relaxations to grammar, e.g. trailing commas and nan/infinity for fp-numbers.

If we want to be user-friendly, perhaps we shouldn't alienate users that have need for comments in their config files?

@nlohmann
Copy link
Owner

nlohmann commented Dec 3, 2016

For this library, we do not plan to support any input which is not compliant to RFC 7159.

See #363, #311, #294, #192.

@nlohmann nlohmann closed this as completed Dec 3, 2016
@nlohmann
Copy link
Owner

nlohmann commented Dec 3, 2016

Another argument (I think) against comment support would be that the library then should preserve comments so that they do not disappear in the serialization.

@TurpentineDistillery
Copy link
Author

Understood. But it the case you decide to support the comments, such support, by definition; should only entail ignoring them in the input; preserving them should definitely not be in scope.

@jschmidt42
Copy link

I agree that it would be nice if the library could ignore comments when parsing JSON. Maybe json::parse could add a parameter flag do allow non-standard additional parsing options. Similar to the options used by this lib as an example: https://github.com/niklasfrykholm/nflibs/blob/master/nf_json_parser.c#L22

@nlohmann
Copy link
Owner

I know how I would realize this feature. But I do think that this library is better off without comments.

@rokups
Copy link

rokups commented Apr 7, 2018

I would like to reiterate that a more liberal parser is a desired feature. Comments are not a standard feature therefore it would be completely acceptable if they were not preserved. This library is my first pick for json parsing, however lack of comment support or even inability to ignore comma after last } is a dealbreaker. I hope enough people will voice their concerns so that this feature can be reconsidered.

@tomba
Copy link

tomba commented Oct 12, 2018

I'm about to start hacking on the library to support trailing commas (although I have no idea where to start looking =) ). When dealing with config json files it drives me nuts that I need to constantly tweak the commas. And comments would make those config files much more readable.

@OvermindDL1
Copy link

I'm about to start hacking on the library to support trailing commas

I thought that was specifically disallowed by the JSON spec?

If you are using them as a config file or so then why not use TOML or HOCON or something?

@tomba
Copy link

tomba commented Oct 12, 2018

I'm about to start hacking on the library to support trailing commas

I thought that was specifically disallowed by the JSON spec?

Could be, but I don't see any harm with that as it still parses json written per the spec. Especially in config files, which are not parsed by anyone else but my application.

If you are using them as a config file or so then why not use TOML or HOCON or something?

Thanks, I wasn't aware of HOCON. I need to have a look. I did try TOML, but I didn't move to it. I can't recall what was the issue, but possibly the main issue was that this json library is great, and I anyway need it for parsing received json data. So having additional library which is more difficult to use than this does not sound good. Especially if the only missing piece is the trailing commas =).

@nlohmann
Copy link
Owner

The harm in relaxing standards is IMHO nicely described in The Harmful Consequences of the Robustness Principle.

@kenjichanhkg
Copy link

I like the library, and was planning to use it to load config files. My company wants to put comments in json file. But it doesn't support/ignore comment, and is stopping me using it.

@nlohmann
Copy link
Owner

You should tell your company that the JSON format does not support comments, and forcing you to do so limits the number of libraries - not only in C++.

@ceztko
Copy link

ceztko commented Jan 10, 2019

For anyone up for a challenge, I suggest inheriting std::istream by wrapping another source std::ifstream (or another std::istream), piping content from source and removing non compliant comments. Be aware: I did once for other purpose and it's not trivial. Good starter point here[1].
Time estimate for this task: 1-2 working days with good confidence in working with STL library internals. Quite more time without.

[1] https://stackoverflow.com/a/12114421/213871

@LinuxRocks2000
Copy link

Seriously, what harm could comments do? The implementation would not harm the library, and it would make JSON a lot more usable, especially in large scale apps with teams of developers who all want to mess with configuration files.

@nlohmann
Copy link
Owner

nlohmann commented Apr 2, 2020

The problem is portability. JSON does not support comments.

@rokups
Copy link

rokups commented Apr 2, 2020

It is not a problem if feature is opt-in.

@nlohmann
Copy link
Owner

nlohmann commented Apr 2, 2020

It is, because then this library would accept more than just JSON. And that JSON+x would not be portable any more. I'm sorry, but https://github.com/nlohmann/json#comments-in-json is all I can add to that.

@rokups
Copy link

rokups commented Apr 2, 2020

This library already accepts more than json.

@nlohmann
Copy link
Owner

nlohmann commented Apr 2, 2020

You mean the formats CBOR, MessagePack, etc.? These are independent of JSON.

@ceztko
Copy link

ceztko commented Apr 2, 2020

I think the library could provide an hook to introduce non standard behaviors (like a way to process tokens in advance and substitute then): that way the library itself would be clean of any "heretic" interpretation. But I understand if the author is not interested, then he should not do it. Also, it would introduce a small performance penalty.

@rokups
Copy link

rokups commented Apr 2, 2020

Anyhow as long as it is opt-in i see no issue with incompatibilities. If people explicitly opt in i trust they understand what they are doing. And this blatant refusal without a compelling argument seems stubborn.

For anyone who wants json with comments: https://github.com/Tencent/rapidjson/

@robiwano
Copy link

robiwano commented May 7, 2020

JSON now has support for comments in the standard. https://json5.org/

@nlohmann
Copy link
Owner

nlohmann commented May 7, 2020

That is not JSON (as of RFC 8259), but slightly related standard.

@robiwano
Copy link

robiwano commented May 7, 2020

Hmm... I realize though that since you use std::map in the DOM, supporting comments would not only not according to RFC 8259, but damn near impossible. Since I need a format that can annotate settings, and be read AND writeable, I might need to find another solution.

@nlohmann
Copy link
Owner

nlohmann commented May 7, 2020

I'm not sure what this has to do with std::map.

@robiwano
Copy link

robiwano commented May 7, 2020

Since it is ordered. How would an implementation know how comments, or annotations, are related to the respective fields? A write could easily destroy this.

@nlohmann
Copy link
Owner

nlohmann commented May 7, 2020

I see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants