Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

Fix: make no-unused-vars not reporting enum members #558

Closed
wants to merge 1 commit into from

Conversation

mysticatea
Copy link
Member

This is a small followup for #553.

Currently, no-unused-vars rule reports enum members.

enum E {
    FOO //→ 'FOO' is assigned a value but never used.
}

This behavior is not nice.

This PR makes to set eslintUsed flag to the enum member variables, as similar to /* exported */ directive comments. As a result, no-unused-vars knows the enum members are exported (used on other places).

@mysticatea mysticatea added the bug label Nov 21, 2018
@platinumazure
Copy link
Member

I wonder if this is the right approach?

My thinking: Enums are sort of like objects, and enum values are sort of like object properties. I don't think we check object properties in no-unused-vars. So I feel that enum values also shouldn't be considered variables and shouldn't be added to a variable scope.

Maybe this isn't possible for other reasons? Let me know.

@mysticatea
Copy link
Member Author

It's better if the enum members behaved like properties. In fact, those close to variables. See also: #552 (comment)

@armano2
Copy link
Contributor

armano2 commented Nov 21, 2018

i'm unsure if this is good way to do it, it will be better if this will be supported by eslint-plugin-typescript?

with actual checking if they are used?

with that we are leaving users with possibility to turn on/off this

@mysticatea
Copy link
Member Author

I think that this approach is fair enough because:

  1. The enum members are exported from the scope of the enum body.
  2. This is the very similar issue to that global variables are exported from the file.
  3. We handle the exported variables by the eslintUsed property.

@mysticatea
Copy link
Member Author

The typescript/use-enum-members rule or something like (as similar to react/jsx-uses-vars) work for me.
But I think nice if it's supported in the parser because the similar /* exported */ directive is supported in core.

ficristo added a commit to quadre-code/quadre that referenced this pull request Dec 6, 2018
no-unused-vars is still turned off for ts, tsx waiting for
eslint/typescript-eslint-parser#558
ficristo added a commit to quadre-code/quadre that referenced this pull request Dec 6, 2018
no-unused-vars is still turned off for ts, tsx waiting for
eslint/typescript-eslint-parser#558
@eventualbuddha
Copy link

This issue is causing my team to sprinkle /* eslint-disable no-unused-vars */ in our codebase whenever we have an enum. I'm happy to help fix this, or to augment typescript/no-unused-vars to handle this. What do y'all think should be done here?

@armano2
Copy link
Contributor

armano2 commented Dec 16, 2018

there is PR in eslint-plugin-typescript with this: bradzacher/eslint-plugin-typescript#236

@JamesHenry
Copy link
Member

@armano2 @bradzacher Will merging this PR interfere with the plugin's implementation in any way?

@bradzacher
Copy link

@JamesHenry - I don't think so. In fact, it means we can delete some code!

https://github.com/bradzacher/eslint-plugin-typescript/blob/c9d2227db2bea041bcd3dc21c639b05b0732d61e/lib/rules/no-unused-vars.js#L63-L65

@kaicataldo
Copy link
Member

Closing this PR since the project has been moved to the TypeScript ESLint organization. Feel free to reopen the PR there.

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

Successfully merging this pull request may close these issues.

7 participants