-
Notifications
You must be signed in to change notification settings - Fork 416
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
Update ESLint configuration for json5 files #2317
base: main
Are you sure you want to change the base?
Conversation
I will restart the discussion on this after DSpace 7.6 gets released. In that time it would be good to think about the irregular space rule and the one blank line rule so we can start enforcing style in CI lint tests. |
553419c
to
1208385
Compare
dbcd5d0
to
7d8bb75
Compare
@alanorth some tests failing but a rebase will probably fix them i imagine. i'll give this a check |
7d8bb75
to
9508498
Compare
Thanks @kshepherd. I've rebased on latest |
], | ||
"rules": { | ||
"no-irregular-whitespace": "error", | ||
// ESLint core no-irregular-whitespace rule doesn't work well in JSON | ||
"no-irregular-whitespace": "off", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might have misunderstood what this rule does, this does not verify if the correct amount of spaces were used (like jsonc/no-irregular-whitespace
), but it checks if any irregular whitespaces were used. An example of one of those irregular whitespaces was for example on line 1061 in fr.json5
in this commit. When I ran that command in the fr.json5
it removed 27 No-Break Spaces. These spaces prevent the text to wrap with text-wrap
so that's why I enabled it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @alexandrevryghem, it's been a while since I looked at this PR. Looking at it now, I see this commit disables eslint's core no-irregular-whitespace
plugin and uses jsonc/no-irregular-whitespace
instead, as recommended by the jsonc plugin. The new plugin is set to error
just like it was previously, but uses some overrides—maybe I can disable those to keep everything the same.
Also, I don't see any non-breaking spaces (U+00A0) in fr.json5
any more on the main
branch? Your commit remove actually removed them.
The eslint-plugin-jsonc has a recommended configuration for json5. See: https://ota-meshi.github.io/eslint-plugin-jsonc/user-guide/#usage
The eslint-plugin-jsonc documentation recommends turning ESLint's own no-irregular-whitespace plugin off for JSON files in favor of its own jsonc/no-irregular-whitespace plugin. The idea here is to allow "irregular" whitespace in quoted strings and regular expressions, as that may be deliberate, and to forbid such whitespace in comments and templates (though I'm unsure if we should skip them in templates?). See: https://ota-meshi.github.io/eslint-plugin-jsonc/rules/no-irregular-whitespace.html
Only allow one blank line in json5 files as well as a max of one line at the end of the file. See: https://eslint.org/docs/latest/rules/no-multiple-empty-lines
We want to avoid this: { // "bitstream.download.page.back": "Back" , "bitstream.download.page.back": "Atrás" , } There is no need for the space before the comma. Note that we do not need this plugin to check for spaces after the comma because those will be highlighted by the other trailing whitespace checks. See: https://eslint.org/docs/latest/rules/comma-spacing
References
Description
Update the ESLint configuration for json5 files to enforce a certain style that we can automatically verify and enforce for all future commits, pull requests, etc.
Note: after merging this, all i18n JSON5 files will need to be fixed using
yarn lint --fix
, or it can be done as part of this.Instructions for Reviewers
List of changes in this PR:
plugin:jsonc/recommended-with-json5
instead ofplugin:jsonc/recommended-with-jsonc
jsonc/no-irregular-whitespace
plugin instead of ESLint'sno-irregular-whitespace
no-multiple-empty-lines
plugin to enforce one blank line in between contentcomma-spacing
plugin to disallow content likethis ,
We need to decide:
no-multiple-empty-lines
is what we want, ie do we want to enforce one blank line as the standard to maintain human readability?yarn lint --fix
to all i18n JSON5 files as part of this pull requestChecklist
yarn lint
yarn check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.