typescript-eslint/typescript-eslintPublic

NotificationsYou must be signed in to change notification settings
Fork2.8k
Star15.8k

fix(eslint-plugin): [no-unused-vars] SelfReferenced should see varsIgnorePattern#2679

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

aggmoulik wants to merge5 commits intotypescript-eslint:masterfromaggmoulik:fix-no-unused-vars

Closed

fix(eslint-plugin): [no-unused-vars] SelfReferenced should see varsIgnorePattern#2679

aggmoulik wants to merge5 commits intotypescript-eslint:masterfromaggmoulik:fix-no-unused-vars

Conversation

Copy link

Contributor

aggmoulik commentedOct 16, 2020•
edited
Loading

Fix:#2648
Signed-off-by: Moulik Aggarwalqwertymoulik@gmail.com

fix(eslint-plugin): [no-unused-vars] SelfReferenced should see varsIg…

035989c

…norePatternSigned-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>

Copy link

Contributor

typescript-eslintbot commentedOct 16, 2020

Thanks for the PR,@aggmoulik!

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 visitorsper day.

Merge branch 'master' into fix-no-unused-vars

b51da23

Copy link

codecovbot commentedOct 16, 2020•
edited
Loading

Codecov Report

Merging#2679 intomaster willdecrease coverage by0.02%.
The diff coverage is25.00%.

@@            Coverage Diff             @@##           master    #2679      +/-   ##==========================================- Coverage   92.82%   92.79%   -0.03%==========================================  Files         294      294                Lines        9670     9674       +4       Branches     2712     2715       +3     ==========================================+ Hits         8976     8977       +1- Misses        328      329       +1- Partials      366      368       +2

Flag	Coverage Δ
#unittest	`92.79% <25.00%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown.Click here to find out more.

Impacted Files	Coverage Δ
packages/eslint-plugin/src/rules/no-unused-vars.ts	`90.56% <25.00%> (-2.58%)`	⬇️

HolgerJeromin reviewed

Oct 16, 2020

View reviewed changes

packages/eslint-plugin/src/rules/no-unused-vars.ts Outdated

		});
		if (
		varsIgnorePattern &&
		varsIgnorePattern.toString() !== variable.name

Copy link

HolgerJerominOct 16, 2020

Choose a reason for hiding this comment

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

varsIgnorePattern is a regexp (or perhaps still a regexp string).

See usage in eslint:
if (config.varsIgnorePattern && config.varsIgnorePattern.test(def.name.name)) {

https://github.com/eslint/eslint/blob/551b1e9fbc463db4869e738a8228868f7323788c/lib/rules/no-unused-vars.js#L590

Copy link

HolgerJerominOct 16, 2020

Choose a reason for hiding this comment

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

And this error should be reported also when novarsIgnorePattern is given.

Copy link

HolgerJerominOct 16, 2020

Choose a reason for hiding this comment

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

Do we need a similar change in line 148?

Copy link

ContributorAuthor

aggmoulikOct 16, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Initially, I was confused on the same thing.
But, I found this as my reference to go for this method:

varvarIgnorePattern:"all"|"local"|{vars?:"all"|"local"|undefined;varIgnorePattern:?:string|undefined;args:"all"|"after-used"|"none"|undefined;  .........caughtErrorsIgnorePattern?:string|undefined;

Copy link

ContributorAuthor

aggmoulikOct 16, 2020

Choose a reason for hiding this comment

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

Doing the regex, I am having an error

varIgnorePattern.test(variable.name)

Copy link

ContributorAuthor

aggmoulikOct 17, 2020

Choose a reason for hiding this comment

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

Hy@HolgerJeromin, Can you guide me ?

Copy link

HolgerJerominOct 17, 2020•
edited
Loading

Choose a reason for hiding this comment

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

No, sorry. I never hacked around around eslint.
I can try to help after my vacation in one week

Copy link

Member

bradzacherOct 18, 2020

Choose a reason for hiding this comment

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

So here you'll need to convert the string to a regexp:

const varIgnorePattern =  typeof options === 'object' && options.varIgnorePattern != null    ? new RegExp(options.varsIgnorePattern, "u")    : null;

Copy link

ContributorAuthor

aggmoulikOct 19, 2020

Choose a reason for hiding this comment

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

I have worked with the regex but how can I improve codecov ?@bradzacher

Copy link

Member

bradzacherOct 19, 2020

Choose a reason for hiding this comment

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

you'll need some tests! right now you have none at all.

bradzacher added bug

Something isn't working

awaiting responseIssues waiting for a reply from the OP or another party labels

Oct 16, 2020

aggmoulik added3 commits

October 19, 2020 11:40

fix(eslint-plugin): [no-unused-vars] Updated using regex patterns

d3e470b

Signed-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>

Merge branch 'master' into fix-no-unused-vars

a570994

fix: resolved lint errors

7f0002a

Signed-off-by: Moulik Aggarwal <qwertymoulik@gmail.com>

bradzacher added a commit that referenced this pull request

Nov 16, 2020

feat(eslint-plugin): [no-unused-vars] fork the base rule

a1478ae

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.Fixes#2714Fixes#2648Closes#2679

bradzacher mentioned this pull request

Nov 16, 2020

feat(eslint-plugin): [no-unused-vars] fork the base rule#2768

Merged

Copy link

Member

bradzacher commentedNov 18, 2020

Closing in favour of#2768

bradzacher closed this

Nov 18, 2020

bradzacher added a commit that referenced this pull request

Nov 22, 2020

feat(eslint-plugin): [no-unused-vars] fork the base rule

8062050

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.Fixes#2714Fixes#2648Closes#2679