-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [no-unused-vars] fork the base rule #2768
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
Conversation
Thanks for the PR, @bradzacher! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
Codecov Report
@@ Coverage Diff @@
## master #2768 +/- ##
==========================================
- Coverage 92.80% 92.76% -0.04%
==========================================
Files 300 301 +1
Lines 9857 10150 +293
Branches 2769 2870 +101
==========================================
+ Hits 9148 9416 +268
- Misses 332 345 +13
- Partials 377 389 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
|
a1478ae
to
8062050
Compare
702650d
to
3478bb1
Compare
3478bb1
to
40e40c4
Compare
Hey 👋🏼 Sorry if this is not a correct channel to ask this, I can raise a separate issue as well, just let me know :) Can you please help out to understand better the relation between this rule, new https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars.md Up to We currently have this, but as I understand we can change it to some combo of post {
// While main rules works, it reports errors in places where TS would not complain, like underscored variables e.g. _actions
// https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars.md
//
// Experimental rule seems to be aligned better with TS itself
// https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars-experimental.md
'no-unused-vars': [OFF],
'@typescript-eslint/no-unused-vars-experimental': [ERROR],
} Seems like documentation for |
My understanding is this: |
This is explained in #1856 |
@@ -4,11 +4,33 @@ import { | |||
TSESTree, | |||
} from '@typescript-eslint/experimental-utils'; | |||
import { PatternVisitor } from '@typescript-eslint/scope-manager'; | |||
import baseRule from 'eslint/lib/rules/no-unused-vars'; | |||
import { getNameLocationInGlobalDirectiveComment } from 'eslint/lib/rules/utils/ast-utils'; |
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.
this line breaks using eslint@5, which is supported:
"eslint": "^5.0.0 || ^6.0.0 || ^7.0.0" |
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.
see jest-community/eslint-plugin-jest#732 for CI failure
Cannot find module 'eslint/lib/rules/utils/ast-utils'
Require stack:
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/@typescript-eslint/eslint-plugin/dist/rules/no-unused-vars.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/@typescript-eslint/eslint-plugin/dist/rules/index.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/@typescript-eslint/eslint-plugin/dist/index.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/eslint/lib/config/plugins.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/eslint/lib/config.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/eslint/lib/cli-engine.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/eslint/lib/api.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/jest-runner-eslint/build/runner/runESLint.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/create-jest-runner/build/createJestRunner.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/create-jest-runner/build/index.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/jest-runner-eslint/build/runner/index.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/@jest/core/build/TestScheduler.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/@jest/core/build/jest.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/jest-cli/build/cli/index.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/jest-cli/bin/jest.js
- /home/runner/work/eslint-plugin-jest/eslint-plugin-jest/node_modules/jest/bin/jest.js
I wanted to avoid doing this, but not doing this restricts our logic too much - it causes problems when we want to consider reporting on things that the base rule wouldn't report on.
By forking this logic - it also opens the door to using this in
naming-convention
to detect unused variables.Fixes #2782
Fixes #2714
Fixes #2648
Closes #2679