-
Notifications
You must be signed in to change notification settings - Fork 82
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 more ESLint recommended rules, and enforce them in CI. #367
Conversation
✅ Deploy Preview for colorjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Hey, thank you so much for your hard work and perseverance on this!
-
Regarding errors vs warnings: I tend to specify all linting as warnings, to distinguish from actual errors.
-
As a general philosophy, I am against
// eslint-disable-*
directives. I think the code should be the source of truth, written in the optimal way, and tools like ESLint should just work with it without leaving traces in the codebase. It should not require more migration to swap ESLint for another linter than migrating the config file itself. If we need to override a rule (rather than refactoring the code) I take it as a strong indication that maybe the rules we have are too strict and we've ended up fighting against them rather than them helping us, so we should not be enforcing the rule in the first place, or we should be enforcing a more lax version of the rule that would allow that kind of formatting. -
It would be good to commit some of the uncontroversial code changes this PR introduces (e.g. adding semicolons, space between func name and opening paren, things like that) in a separate PR so that this would not be as huge to review. In fact, I'd suggest this workflow:
- Take all code changes in here. Remove those that require eslint overrides, and those that clearly do not improve readability.
- Send a PR with just the code changes, without the linting infrastructure.
- Once we iterate and merge that, then we can focus on any changes the linting infrastructure introduces without being lost in the hundreds of missing semicolons added and the like.
How does that sound?
.eslintrc.json
Outdated
@@ -11,27 +12,71 @@ | |||
"browser": true, | |||
"node": true | |||
}, | |||
"globals": { "Proxy": "readonly" }, | |||
"extends": [ | |||
"eslint:recommended", |
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’m a little uncomfortable with leaving coding style up to mystery bags of rules that we have little control over, and don't fully realize what we're enforcing. Could we list the rules explicitly?
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'm torn on this. On one hand I understand wanting the rules to be explicit and opt-in; on the other hand ESLint is very stable, and maintains a long list of recommended rules that don't change often and would be duplicated here. And typescript-eslint
also disables some of the default ESLint rules that it replaces, so it would be a very long list. I'm open to copying them here if you prefer that (and we could try narrowing them down). The lists this is currently using are:
- https://github.com/eslint/eslint/blob/v8.54.0/packages/js/src/configs/eslint-recommended.js
- https://github.com/typescript-eslint/typescript-eslint/blob/v6.13.1/packages/eslint-plugin/src/configs/eslint-recommended.ts
- https://github.com/typescript-eslint/typescript-eslint/blob/v6.13.1/packages/eslint-plugin/src/configs/recommended.ts
- https://github.com/typescript-eslint/typescript-eslint/blob/v6.13.1/packages/eslint-plugin/src/configs/stylistic.ts
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.
Continued in #373
This reverts commit 82d2ce9.
@LeaVerou I pulled out the code changes into #372, and rebased this PR off of that one. |
A replacement of #351. Based off of #372.
A few notes/questions:
Very open to feedback and/or rules changes!