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 jsonc format #457

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Support jsonc format #457

wants to merge 4 commits into from

Conversation

up9cloud
Copy link
Contributor

I did a new commit, see #452

Signed-off-by: T.C. <8325632+up9cloud@users.noreply.github.com>
Signed-off-by: T.C. <8325632+up9cloud@users.noreply.github.com>
Copy link
Collaborator

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some remarks. Also, the two commits could be squashed into one commit,... as well as further fixups of course!

Either way thanks for your contribution!

@@ -52,7 +52,7 @@ fn test_file() {
assert_eq!(s.place.telephone, None);
assert_eq!(s.elements.len(), 10);
assert_eq!(s.elements[3], "4".to_string());
if cfg!(feature = "preserve_order") {
if cfg!(feature = "TODO: preserve_order") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, that's because jsonc-parser doesn't support preserve_order. need time to contribute that

Comment on lines +91 to +95
#[cfg(all(feature = "jsonc", feature = "json"))]
formats.insert(FileFormat::Jsonc, vec!["jsonc"]);

#[cfg(all(feature = "jsonc", not(feature = "json")))]
formats.insert(FileFormat::Jsonc, vec!["jsonc", "json"]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that the jsonc parser also parses json even if that feature is disabled?

I think that's counter-intuitive and we should only parse jsonc with the jsonc parser.

Copy link
Contributor Author

@up9cloud up9cloud Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, there are 2 reasons i added that.

  1. the file extension for jsonc is actually .json, not .jsonc, so the real question is that when i enable the feature jsonc it's about the format or the file extension?
  2. for the common usage, people would choose only one of them, not both. and I believe the jsonc format is more common for config files than the json format.

@matthiasbeyer matthiasbeyer added the hacktoberfest-accepted Accepted PR for hacktoberfest label Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted PR for hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants