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

Close #3872 - Add eslint-plugin-jsonc as a linter for JSON, JSONC and JSON5 #3873

Merged
merged 7 commits into from
Aug 21, 2021

Conversation

jpesce
Copy link
Contributor

@jpesce jpesce commented Aug 14, 2021

Use the same lint configuration as eslint for javascript in JSON, JSONC and JSON5 files. Requires eslint-plugin-jsonc in .eslintrc.

This is the first time working with the codebase, so let me know if there's an easier way to reuse the eslint configuration without having to copy everything. :)

If you think the PR makes sense, I'll try to write the tests.

Closes #3872

Use the same lint configuration as eslint for javascript.
@jpesce jpesce changed the title WIP - Fix #3872 - Add eslint-plugin-jsonc as a linter for JSON, JSONC and JSON5 WIP - Close #3872 - Add eslint-plugin-jsonc as a linter for JSON, JSONC and JSON5 Aug 14, 2021
@hsanson
Copy link
Contributor

hsanson commented Aug 17, 2021

Looks good. You are already reusing well the code for eslint and just defining the linter for additional languages. The only problem I see is that this would enable the eslint linter for JSON, JSONC, and JSON5 files even if the plugin is not present.

Ensure to update the list of supported tools in supported-tools.md, ale-supported-languages-and-tools.txt, and add entries to ale.txt for the newly supported languages. See ale-graphql-eslint for example.

@jpesce
Copy link
Contributor Author

jpesce commented Aug 18, 2021

Ensure to update the list of supported tools in supported-tools.md, ale-supported-languages-and-tools.txt, and add entries to ale.txt for the newly supported languages. See ale-graphql-eslint for example.

Updated 🐸

Remove any preference for eslint plugins, since there are more thant one
that would work
@hsanson
Copy link
Contributor

hsanson commented Aug 19, 2021

@jpesce sorry I forgot to mention that the list of supported linters must be sorted alphabetically or the tests would fail. This is the correct sorting:

* JSON
  * [eslint](http://eslint.org/)
  * [fixjson](https://github.com/rhysd/fixjson)
  * [jq](https://stedolan.github.io/jq/)
  * [jsonlint](https://github.com/zaach/jsonlint)
  * [prettier](https://github.com/prettier/prettier)
  * [spectral](https://github.com/stoplightio/spectral)
* JSON5
  * [eslint](http://eslint.org/)
* JSONC
  * [eslint](http://eslint.org/)
  1. JSON5 section must be before JSONC
  2. eslint in the JSON section must be the first entry.
  3. This must be true on both supported-tools.md and ale-supported-languages-and-tools.txt files.

@jpesce
Copy link
Contributor Author

jpesce commented Aug 19, 2021

@jpesce sorry I forgot to mention that the list of supported linters must be sorted alphabetically or the tests would fail.

Makes sense, thanks! Updated them 🐸

@jpesce
Copy link
Contributor Author

jpesce commented Aug 19, 2021

Sorry, there was a misalignment in ale.txt

Fixed it in d0156e6

@hsanson
Copy link
Contributor

hsanson commented Aug 20, 2021

Test still failing... I think because the order within ale.txt must also match the order in supported tools.

If you have docker daemon would be good to run the tests locally before pushing the chages:

./run-tests -v --linters-only

@jpesce
Copy link
Contributor Author

jpesce commented Aug 20, 2021

Sorry about that, I installed docker and ran the tests and now it's passing.

One remark though — the test expects the following order in ale.txt:

  1. json
  2. jsonc
  3. json5

but it wierdly expects json5 to be before jsonc in ale-supported-languages-and-tools.txt, like so:

  1. json
  2. json5
  3. jsonc
Here's the test output
./run-tests -v --linters-only
Docker run image denseanalysis/ale:180c2d67c6a055af98b836ae9d28e3db ready
Starting Vint...
Starting Custom checks...

========================================
Running Vint to lint our code
========================================
Vint warnings/errors follow:


========================================
Running custom linting rules
========================================
Custom warnings/errors follow:


========================================
Checking for duplicate tags
========================================
Duplicate tags follow:


========================================
Checking for invalid tag references
========================================
Invalid tag references tags follow:

========================================
diff supported-tools.md and doc/ale-supported-languages-and-tools.txt tables
========================================
Differences follow:

========================================
Look for badly aligned doc tags
========================================
Badly aligned tags follow:

========================================
Look for table of contents issues
========================================

Check for bad ToC sorting:

Check for mismatched ToC and headings:

========================================
Check Python code
========================================

.....
----------------------------------------------------------------------
Ran 5 tests in 0.002s

OK

All tests passed!

Let me know how you prefer to continue 🐸
Thanks!

@hsanson hsanson changed the title WIP - Close #3872 - Add eslint-plugin-jsonc as a linter for JSON, JSONC and JSON5 Close #3872 - Add eslint-plugin-jsonc as a linter for JSON, JSONC and JSON5 Aug 21, 2021
Copy link
Contributor

@hsanson hsanson left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and patience.

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.

Add support for eslint-plugin-jsonc plugin
3 participants