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

feat: Add allowTrailingCommas option for JSONC #42

Merged
merged 4 commits into from
Oct 25, 2024
Merged

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Oct 16, 2024

Prerequisites checklist

What is the purpose of this pull request?

Add support for trailing commas to the JSONC language.

What changes did you make? (Give an overview)

This pull request introduces support for allowing trailing commas in JSONC files and includes several updates to the codebase and documentation to reflect this new feature. The most important changes include adding a new option to enable trailing commas, updating the JSONLanguage class to validate and handle this option, and adding tests to ensure the feature works as expected.

New Feature: Allow Trailing Commas in JSONC

  • README.md: Added a section explaining how to enable trailing commas in JSONC files using the allowTrailingCommas language option.

Code Updates

  • src/languages/json-language.js:
    • Added a new typedef for JSONLanguageOptions to include the allowTrailingCommas property.
    • Updated the validateLanguageOptions method to check for the allowTrailingCommas option and ensure it is only used in JSONC mode.
    • Modified the parse method to accept and handle the allowTrailingCommas option.

Dependency Update

  • package.json: Updated the @humanwhocodes/momoa dependency to version ^3.3.0.

Tests

  • tests/languages/json-language.test.js:
    • Added tests for the validateLanguageOptions method to ensure it correctly handles the allowTrailingCommas option.
    • Added tests for the parse method to verify that trailing commas are correctly parsed or rejected based on the allowTrailingCommas option.

Related Issues

fixes #41

Is there anything you'd like reviewers to focus on?

Right now, validateLanguageOptions() throws an error when it finds allowTrailingCommas in JSON mode (where it cannot be enabled) and JSON5 mode (where it cannot be disabled). Would it be better to check the value of allowTrailingCommas before deciding to throw an error? So in JSON mode it would only throw if allowTrailingCommas is true and in JSON5 mode it would only throw an error if allowTrailingCommas is false?

@mdjermanovic
Copy link
Member

Right now, validateLanguageOptions() throws an error when it finds allowTrailingCommas in JSON mode (where it cannot be enabled) and JSON5 mode (where it cannot be disabled). Would it be better to check the value of allowTrailingCommas before deciding to throw an error? So in JSON mode it would only throw if allowTrailingCommas is true and in JSON5 mode it would only throw an error if allowTrailingCommas is false?

I think it's fine to throw an error when this option is used in a mode where it isn't applicable, regardless of the value.

README.md Outdated Show resolved Hide resolved
@fasttime fasttime mentioned this pull request Oct 25, 2024
1 task
@fasttime
Copy link
Member

@nzakas There's a check failing because of a formatting problem reported by Prettier. Can you take a look?

@nzakas
Copy link
Member Author

nzakas commented Oct 25, 2024

Hmm, I'm not seeing anything locally. 🤔

@nzakas
Copy link
Member Author

nzakas commented Oct 25, 2024

Rebasing worked. 👍

@mdjermanovic
Copy link
Member

Sorry, I left a suggestion to fix the prettier error last week, by apparently forgot to click the submit review button.

README.md Outdated Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <[email protected]>
README.md Outdated Show resolved Hide resolved
Co-authored-by: Milos Djermanovic <[email protected]>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit c94953b into main Oct 25, 2024
15 checks passed
@mdjermanovic mdjermanovic deleted the issue41 branch October 25, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Dangling commas in JSONC
3 participants