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

Add more ESLint recommended rules, and enforce them in CI. #367

Closed
wants to merge 1 commit into from

Conversation

jgerigmeyer
Copy link
Member

@jgerigmeyer jgerigmeyer commented Nov 28, 2023

A replacement of #351. Based off of #372.

A few notes/questions:

  1. What is the value of "warnings" vs "errors", and what should cause CI to fail? Right now the default recommended rules are set to "error" (except where overridden), but most of the custom rules are set to "warn". CI will fail if there are any errors or warnings. I'd probably recommend moving all of them to "error" and not using "warn" at all -- it sort of defeats the purpose if warnings pile up unaddressed, and can easily mask newly introduced unintentional warnings.
  2. There are a handful of places where rules are enabled globally (e.g. no-cond-assign, no-sparse-arrays, no-redeclare, no-unused-vars) because they're helpful in revealing common typos, but then are overridden in specific instances where the usage is intentional. There are other rules (e.g. no-loss-of-precision) that we might want to disable globally?

Very open to feedback and/or rules changes!

Copy link

netlify bot commented Nov 28, 2023

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit aac6e62
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/65677bbb20b3a10009f68818
😎 Deploy Preview https://deploy-preview-367--colorjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

.editorconfig Outdated Show resolved Hide resolved
Copy link
Member

@LeaVerou LeaVerou left a 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!

  1. Regarding errors vs warnings: I tend to specify all linting as warnings, to distinguish from actual errors.

  2. 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.

  3. 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:

    1. Take all code changes in here. Remove those that require eslint overrides, and those that clearly do not improve readability.
    2. Send a PR with just the code changes, without the linting infrastructure.
    3. 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 Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
.eslintrc.json Outdated
@@ -11,27 +12,71 @@
"browser": true,
"node": true
},
"globals": { "Proxy": "readonly" },
"extends": [
"eslint:recommended",
Copy link
Member

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?

Copy link
Member Author

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:

Copy link
Member Author

Choose a reason for hiding this comment

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

Continued in #373

.eslintrc.json Outdated Show resolved Hide resolved
notebook/repl.js Outdated Show resolved Hide resolved
src/color.js Outdated Show resolved Hide resolved
src/equals.js Outdated Show resolved Hide resolved
src/hooks.js Outdated Show resolved Hide resolved
src/serialize.js Outdated Show resolved Hide resolved
src/space-accessors.js Outdated Show resolved Hide resolved
@jgerigmeyer jgerigmeyer changed the base branch from main to fix-formatting November 29, 2023 18:00
@jgerigmeyer
Copy link
Member Author

  1. Take all code changes in here. Remove those that require eslint overrides, and those that clearly do not improve readability.
  2. Send a PR with just the code changes, without the linting infrastructure.
  3. 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?

@LeaVerou I pulled out the code changes into #372, and rebased this PR off of that one.

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.

None yet

2 participants