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

Support overrides #317

Closed
MichaelDeBoey opened this issue Apr 14, 2020 · 14 comments · Fixed by #343
Closed

Support overrides #317

MichaelDeBoey opened this issue Apr 14, 2020 · 14 comments · Fixed by #343

Comments

@MichaelDeBoey
Copy link
Contributor

MichaelDeBoey commented Apr 14, 2020

When adding TypeScript support in eslint-config-kentcdodds (see kentcdodds/eslint-config-kentcdodds#54), I noticed CI isn’t failing even though I haven't added all @typescript-eslint rules (yet).

I'm happy to help out if you could point me into the right direction of adding support here. 🙂

@ljharb
Copy link
Collaborator

ljharb commented Apr 15, 2020

The intention is generally for all rules to be defined for every file; in your case, you could define them all at the root, and only enable them in the overrides?

@MichaelDeBoey
Copy link
Contributor Author

@ljharb That would be really cumbersome to duplicate all rules.
1 time for just having it there (disabled) and make eslint-find-rules happy and 1 time to enable them.

For now it's only for @typescript-eslint rules, but this can become more in the future.

Like I said: I'm happy to implement it myself if you could point me in the right direction. 🙂

@ljharb
Copy link
Collaborator

ljharb commented Apr 17, 2020

@MichaelDeBoey the airbnb config defines every single rule in eslint core, eslint-plugin-import, eslint-plugin-react, and eslint-plugin-jsx-a11y. It's a one-time cost that eslint-find-rules helps keep up to date with minimal incremental effort.

In your case, you could easily define them once in a JS object, and then add that object in two places: the root, off; and overrides, as "error".

@kentcdodds
Copy link
Collaborator

Hi @ljharb 👋 It's been a while! Hope you're doing awesome.

Are you opposed to someone adding support for this? Seems like what you're suggesting is more of a workaround than a desirable "feature" 🙃

@ljharb
Copy link
Collaborator

ljharb commented Apr 19, 2020

@kentcdodds to be specific, it seems like what you're asking is for --unused to also check for the presence of rules in overrides?

Based on the existence of --include=deprecated, it seems reasonable to allow for providing something like --consider=overrides, but I'd still encourage you to define every rule for every file, to comply with the spirit of the "unused" check.

@kentcdodds
Copy link
Collaborator

That makes sense :) I'd definitely use a --consider=overrides flag and I'm ok with that being an opt-in thing personally :)

@MichaelDeBoey
Copy link
Contributor Author

@ljharb Could you point me into the right direction please?
I'd love to work on this.

@ljharb
Copy link
Collaborator

ljharb commented May 5, 2020

I'm not entirely sure :-) i'd start with tests tho

@saiichihashimoto
Copy link

@MichaelDeBoey did you ever figure this out?

@MichaelDeBoey
Copy link
Contributor Author

@saiichihashimoto I never had the time to look into this.
If you want to, be my guest! 🙂

@leepowelldev
Copy link

I spent some time on this today, and have it working - ESLint v7 added better support for merging in override rules when building the config. The downside is it would require v7 as the lowest peer dependency to support as CLIEngine is now deprecated. Also it doesn't seem to pickup any plugins that are defined in an overrides block - not sure if this is expected behaviour of ESLint.

@MichaelDeBoey
Copy link
Contributor Author

A new use-case we have on eslint-config-kentcdodds is wanting to enable all Jest related rules only for testing files (as can be seen in kentcdodds/eslint-config-kentcdodds#98).

Enabling all the rules in the root just to make our tests happy isn't the preferred way, so this feature could benefit us a lot.

@leepowelldev Do you have the time to create a PR with working version of this or should we look into it ourselves?

darekkay added a commit to darekkay/darekkay-eslint-config that referenced this issue May 13, 2021
Overrides are not supported, yet, so some rules were missed. See: sarbbottam/eslint-find-rules#317
@darekkay
Copy link

My workaround is to define high-level configurations (containing overrides) and move all plugin rules into separate files:

root
  - config-all.js
  - react.js
  - nodejs.js
  - plugins
    - base.js
    - jest.js
    - typescript.js

So while react.js uses overrides to apply typescript rules only to TS(X) files, there's a special config file config-all.js that contains all rules without overrides:

module.exports = {
  extends: [
    "../plugins/base.js",
    "../plugins/jest.js",
    "../plugins/typescript.js",
  ],

  plugins: ["@typescript-eslint"],
};

When checking config-all.js, eslint-find-rules will now check all referenced plugins/rules.

@MichaelDeBoey
Copy link
Contributor Author

MichaelDeBoey commented May 13, 2021

@darekkay This comes down to the same proposal @ljharb did, which is not really preferred in our use-case.

Thanks for the proposal though! 👊

ljharb added a commit to nicolaichuk/eslint-find-rules that referenced this issue Jan 5, 2022
Fixes sarbbottam#317.

Co-authored-by: nicolaichuk <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
ljharb added a commit to nicolaichuk/eslint-find-rules that referenced this issue Jan 5, 2022
Fixes sarbbottam#317.

Co-authored-by: nicolaichuk <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
@ljharb ljharb closed this as completed in 6dabfb7 Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants