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

Optional comment stripping. #1024

Closed
wants to merge 2 commits into from

Conversation

BrunoFiligree
Copy link

I've added optional code to strip javascript '//' style comments from a json file. This is only enabled if the JSON_STRIP_COMMENTS C++ macro is defined. By default this does not break conformance with the JSON standard, but allows cases where comment stripping is needed to be enabled on an ad-hoc basis without having to introduce pre-parsing phases with any associated extra code.

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Mar 26, 2018
@nlohmann
Copy link
Owner

I don’t think this is a good idea, but I would like to wait for a discussion.

@BrunoFiligree
Copy link
Author

We have use cases where we have end-user editable JSON config files. Comments are required in the files for clarity, rather than have separate documentation that no-one will reference. The JSON standard may state 'no comments', but the current 'best practice' to deal with comments is to run it through a pre-parser. This basically acknowledges that comments will be embedded in some JSON files and those need to be dealt with it on an ad-hoc basis.

Rather than introduce yet another thing into our tool chain to do the ad-hoc pre-parsing comment stripping, it is more efficient all round to put the comment stripping directly into the JSON lexer and enable it as needed.

@nlohmann
Copy link
Owner

Regardless of the outcome of the discussion, please also add test cases.

@BrunoFiligree
Copy link
Author

OK. That may take a day or three. Unit tests always take longer than coding :-)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 72e556e on FiligreeTech:develop into d2dd27d on nlohmann:develop.

@dandee
Copy link

dandee commented Mar 26, 2018

I also think that it is not a good idea to break clearly defined standards: there's nothing about C++ style comments in the reference for JSON.

@nlohmann nlohmann mentioned this pull request Mar 27, 2018
@danielytics
Copy link

They're also javascript-style comments, so not really too far from json. Since its enabled/disabled through a macro, users can choose if they want it or not, so I don't see it as a big deal. Yes, its not "technically" json, but its pure-json by default and is a super useful feature given how often json is used as a configuration format. I don't mind either way though, I don't personally use json for config files.

@nlohmann
Copy link
Owner

nlohmann commented Apr 2, 2018

I still do not like this library to accept input that does not conforms to RFC 7159. I agree with Douglas Crockford:

Suppose you are using JSON to keep configuration files, which you would like to annotate. Go ahead and insert all the comments you like. Then pipe it through JSMin before handing it to your JSON parser.

That parser should not be the one of this library.

@nlohmann nlohmann closed this Apr 2, 2018
@BrunoFiligree
Copy link
Author

Fine. I'll keep my fork.

@nlohmann nlohmann removed the state: please discuss please discuss the issue or vote for your favorite option label Apr 3, 2018
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.

6 participants