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

Add ignore_trailing_commas option #4609

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

chirsz-ever
Copy link

@chirsz-ever chirsz-ever commented Jan 19, 2025

This PR adds ignore_trailing_commas argument to the detail::parser constructor, basic_json::accept, basic_json::parse and basic_json::sax_parse.

Why

In short: I totally agree with the JWCC extension.

After adding this, we can parse tsconfig.json, which allow trailing commas. (By the way, VSCode has a allowTrailingCommas option in its json schema configuration).

More projects would be attracted to use this library, e.g. R2Northstar/NorthstarLauncher#715.

Related issues: #1429 #1787 #1734

Detail Changes

  • Add ignore_trailing_commas argument to the detail::parser constructor, basic_json::accept, basic_json::parse and basic_json::sax_parse.
  • Update documets and README.md.
  • Update the unit test "class_parser". Add , to non-empty objects and arrays for testing.

Pull request checklist

  • The changes are described in detail, both the what and why.
  • If applicable, an existing issue is referenced.
  • The Code coverage remained at 100%. A test case for every new line of code.
  • If applicable, the documentation is updated.
  • The source code is amalgamated by running make amalgamate.

@chirsz-ever chirsz-ever requested a review from nlohmann as a code owner January 19, 2025 16:48
@chirsz-ever chirsz-ever force-pushed the chirsz/feat-allow-trailing-commas-2501 branch from 7c7adb3 to ed1d330 Compare January 19, 2025 16:51
@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Jan 19, 2025
@coveralls
Copy link

coveralls commented Jan 19, 2025

Coverage Status

coverage: 99.186%. remained the same
when pulling 36c5061 on chirsz-ever:chirsz/feat-allow-trailing-commas-2501
into bf8ccc2 on nlohmann:develop.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/features/trailing_commas.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/features/trailing_commas.md Outdated Show resolved Hide resolved
@chirsz-ever chirsz-ever force-pushed the chirsz/feat-allow-trailing-commas-2501 branch from 31dcd16 to e0dfccd Compare January 20, 2025 16:57
docs/mkdocs/docs/features/trailing_commas.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@nlohmann
Copy link
Owner

Maybe the documentation should somewhere also mention JWCC - it's good when things have a name.

@chirsz-ever
Copy link
Author

How do you think about combining the two options ignore_comments and ignore_trailing_commas into a single option to replace the current ignore_comments? For example, after the update, parse(str, nullptr, true, true) would allow both comments and trailing commas.

We can name the new combined option allow_comments_and_trailing_commas or enable_jwcc or relaxed_syntax or another better name.

I think that any user who choose to allow comments will not mind allowing trailing commas, and it is hard to imagine a use case where one must be enabled and the other disabled.

Regarding compatibility issues, this will not affect the serialization results, nor will it refuse to accept inputs accepted by previous versions. It will just accept some inputs that would not have been accepted before.

This solution is also conducive to adding new features in the future. But for me this is not important, because I only support adding two features of JWCC.

@chirsz-ever
Copy link
Author

Maybe the documentation should somewhere also mention JWCC - it's good when things have a name.

Can I declare that this library would never add extensions (such as allow NaN as number, single quoted string, unquoted identifiers as object keys ...) other than JWCC in the future?

@nlohmann
Copy link
Owner

Maybe the documentation should somewhere also mention JWCC - it's good when things have a name.

Can I declare that this library would never add extensions (such as allow NaN as number, single quoted string, unquoted identifiers as object keys ...) other than JWCC in the future?

Never say never...

@nlohmann
Copy link
Owner

How do you think about combining the two options ignore_comments and ignore_trailing_commas into a single option to replace the current ignore_comments? For example, after the update, parse(str, nullptr, true, true) would allow both comments and trailing commas.

We can name the new combined option allow_comments_and_trailing_commas or enable_jwcc or relaxed_syntax or another better name.

I think that any user who choose to allow comments will not mind allowing trailing commas, and it is hard to imagine a use case where one must be enabled and the other disabled.

Regarding compatibility issues, this will not affect the serialization results, nor will it refuse to accept inputs accepted by previous versions. It will just accept some inputs that would not have been accepted before.

This solution is also conducive to adding new features in the future. But for me this is not important, because I only support adding two features of JWCC.

No, please leave it as is as it would change the behavior of existing code. Just like with the dump function, we need to figure out how to organize the great number of parameters in the future.

docs/mkdocs/docs/examples/comments.cpp Outdated Show resolved Hide resolved
docs/mkdocs/docs/examples/trailing_commas.cpp Outdated Show resolved Hide resolved
docs/mkdocs/docs/features/comments.md Show resolved Hide resolved
docs/mkdocs/docs/features/trailing_commas.md Show resolved Hide resolved
@chirsz-ever
Copy link
Author

I combine the documentation about comments and trailing commas. Any more suggestions?

@nlohmann
Copy link
Owner

I combine the documentation about comments and trailing commas. Any more suggestions?

No, please leave it as is. We may combine this in the future, but right now, I would like the URLs of the documentation to not change (https://www.w3.org/Provider/Style/URI).

@chirsz-ever
Copy link
Author

I combine the documentation about comments and trailing commas. Any more suggestions?

No, please leave it as is. We may combine this in the future, but right now, I would like the URLs of the documentation to not change (https://www.w3.org/Provider/Style/URI).

Could we just add a redirect rule to docs/mkdocs/mkdocs.yml as below?

       redirect_maps:
         'api/basic_json/operator_gtgt.md': api/operator_gtgt.md
         'api/basic_json/operator_ltlt.md': api/operator_ltlt.md
         'api/basic_json/operator_literal_json.md': api/operator_literal_json.md
         'api/basic_json/operator_literal_json_pointer.md': api/operator_literal_json_pointer.md
         'api/json_pointer/operator_string.md': api/json_pointer/operator_string_t.md
         'home/code_of_conduct.md': community/code_of_conduct.md
+        'features/comments.md': features/comments_and_trailing_commas.md

@nlohmann
Copy link
Owner

I combine the documentation about comments and trailing commas. Any more suggestions?

No, please leave it as is. We may combine this in the future, but right now, I would like the URLs of the documentation to not change (https://www.w3.org/Provider/Style/URI).

Could we just add a redirect rule to docs/mkdocs/mkdocs.yml as below?

       redirect_maps:
         'api/basic_json/operator_gtgt.md': api/operator_gtgt.md
         'api/basic_json/operator_ltlt.md': api/operator_ltlt.md
         'api/basic_json/operator_literal_json.md': api/operator_literal_json.md
         'api/basic_json/operator_literal_json_pointer.md': api/operator_literal_json_pointer.md
         'api/json_pointer/operator_string.md': api/json_pointer/operator_string_t.md
         'home/code_of_conduct.md': community/code_of_conduct.md
+        'features/comments.md': features/comments_and_trailing_commas.md

Yes, but please revert anyway.

Added examples and modified the corresponding documents and unit tests.

Signed-off-by: chirsz-ever <[email protected]>
@chirsz-ever chirsz-ever force-pushed the chirsz/feat-allow-trailing-commas-2501 branch from 9cc35a4 to 36c5061 Compare January 23, 2025 13:52
@chirsz-ever chirsz-ever changed the title Add allow_trailing_commas option Add ignore_trailing_commas option Jan 23, 2025
@nlohmann nlohmann added review needed It would be great if someone could review the proposed changes. and removed state: please discuss please discuss the issue or vote for your favorite option labels Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation L review needed It would be great if someone could review the proposed changes. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants