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

[New] check unused rules with overrides #343

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

nicolaichuk
Copy link
Contributor

@nicolaichuk nicolaichuk commented Nov 17, 2021

Fixes #317.

Fixes sarbbottam#317.

Co-authored-by: nicolaichuk <[email protected]>
Co-authored-by: Jordan Harband <[email protected]>
Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Can you help me understand what this PR is doing? Would, for example, @scope/scoped-plugin/foo-rule be included in the unused rules list without this fix?

.gitignore Outdated Show resolved Hide resolved
src/lib/rule-finder.js Outdated Show resolved Hide resolved
src/lib/rule-finder.js Outdated Show resolved Hide resolved
@ljharb ljharb changed the title Fix: check unused rules with ovirrides [New] check unused rules with overrides Nov 18, 2021
@MichaelDeBoey
Copy link
Contributor

@nicolaichuk Is there something I can help with in order to get this one merged?

@ljharb
Copy link
Collaborator

ljharb commented Jan 5, 2022

Answering the question i asked above would help.

@MichaelDeBoey
Copy link
Contributor

MichaelDeBoey commented Jan 5, 2022

@ljharb This PR will make it possible to have an .eslintrc like

module.exports = {
  rules: {},
  overrides: {
    files: ['**/__tests__/**/*.+(js|ts)?(x)'],
    rules: {
      'jest/no-conditional-expect': 'error',
    },
  },
}

instead of needing to first disable it

module.exports = {
  rules: {
     'jest/no-conditional-expect': 'off',
  },
  overrides: {
    files: ['**/__tests__/**/*.+(js|ts)?(x)'],
    rules: {
      'jest/no-conditional-expect': 'error',
    },
  },
}

And still have --unused flag pass.

This will make the jest config of eslint-config-kentcdodds doesn't have to use a workaround
https://github.com/kentcdodds/eslint-config-kentcdodds/blob/ef7069e100e932be165115f1d03b5d76a797cf04/jest.js#L137-L141

@ljharb
Copy link
Collaborator

ljharb commented Jan 5, 2022

Great, thanks for explaining.

@ljharb ljharb force-pushed the unused_rules_with_ovirrides branch 2 times, most recently from 2f73651 to 097f203 Compare January 5, 2022 21:07
@ljharb ljharb force-pushed the unused_rules_with_ovirrides branch from 097f203 to 6dabfb7 Compare January 5, 2022 21:21
@codecov-commenter
Copy link

codecov-commenter commented Jan 5, 2022

Codecov Report

Merging #343 (6dabfb7) into master (bc5a855) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #343   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          185       185           
=========================================
  Hits           185       185           
Impacted Files Coverage Δ
src/lib/rule-finder.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc5a855...6dabfb7. Read the comment docs.

@ljharb ljharb merged commit 6dabfb7 into sarbbottam:master Jan 5, 2022
@MichaelDeBoey
Copy link
Contributor

@ljharb Thanks for merging!

Can you please cut a release?

@ljharb
Copy link
Collaborator

ljharb commented Jan 5, 2022

That takes a bit longer due to requiring a review from another collaborator.

@MichaelDeBoey
Copy link
Contributor

Please ping me when it's released

@ljharb
Copy link
Collaborator

ljharb commented Jan 5, 2022

v4.1.0 is released

@nicolaichuk
Copy link
Contributor Author

@ljharb, @MichaelDeBoey

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support overrides
4 participants