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

fix: false positive of no-shadow rule for TypeScript #666

Merged
merged 1 commit into from
Jun 16, 2023
Merged

fix: false positive of no-shadow rule for TypeScript #666

merged 1 commit into from
Jun 16, 2023

Conversation

a-takamin
Copy link
Contributor

I have added code that avoids the "false positive" problem of the eslint/no-shadow rule for TypeScript code discussed in "typescript-eslint" issue below.

Please see this issue for more detailed information.
typescript-eslint/typescript-eslint#2483

@koba04
Copy link
Contributor

koba04 commented May 29, 2023

@a-takamin
Thank you!
I have a question. why do we have to put the settings into the overrides section?
Is there any problems if we put the settings into the top rules section directly in lib/typescript.js rather than wrapping it with the overrides section?

  • lib/typescript.js
"rules": {
  ...
  "no-shadow": "off",
  "@typescript-eslint/no-shadow": "error",
}

@a-takamin
Copy link
Contributor Author

@koba04
Thank you for your comment.
You are right. My change is redundant, and we can put the setting into the top rules section directly.

I should change the content of the pull request. May I change this commit and use force push?

@a-takamin
Copy link
Contributor Author

@koba04
Hello! I have deleted "overrides" section and pushed it.
I would appreciate if you could review the updated pull request. Thank you in advance!

Copy link
Contributor

@koba04 koba04 left a comment

Choose a reason for hiding this comment

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

@a-takamin Thank you! I left a comment.

Comment on lines 5 to 6
"no-shadow": "off",
"@typescript-eslint/no-shadow": "error",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove this change in this file?
This file should only include rules for React & TypeScript and the no-shadow rule is related to TypeScript.
We expect lib/react-typescript is used with lib/typescript, so we don't have to have this setting besides in lib/typescript.

We expect that lib/* is an internal library and presets/* is a public interface, so we don't intend lib/* is used directly in ESLint configs.

Copy link
Contributor Author

@a-takamin a-takamin Jun 6, 2023

Choose a reason for hiding this comment

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

@koba04
Thanks for the review!
I am sorry for the lack of understanding. I understand the difference between lib/* and presets/*. I removed the changes in the file, and could you review it again please?

Copy link
Contributor

@koba04 koba04 left a comment

Choose a reason for hiding this comment

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

👍 Great work! Thank you!

@koba04 koba04 merged commit 619bd74 into cybozu:master Jun 16, 2023
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.

2 participants