Skip to content

feat(eslint-plugin): add "no-enum-literals" rule #315

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

Closed

Conversation

mohsen1
Copy link
Contributor

@mohsen1 mohsen1 commented Feb 24, 2019

@mohsen1 mohsen1 force-pushed the mohsen1--no-enum-literals branch from 7da7895 to 564d479 Compare February 25, 2019 16:51
@mohsen1 mohsen1 force-pushed the mohsen1--no-enum-literals branch from 564d479 to 3cabb56 Compare February 25, 2019 17:18
Mohsen Azimi added 2 commits March 8, 2019 09:31
Make test code multiline
Remove unused schema and defaultOption
@mohsen1 mohsen1 force-pushed the mohsen1--no-enum-literals branch from 004720b to df08d28 Compare March 8, 2019 15:03
bradzacher
bradzacher previously approved these changes Mar 8, 2019
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

LGTM

@mysticatea
Copy link
Member

I wonder if a discussion about naming is needed, because I'm not sure what enum-literals is.

@bradzacher
Copy link
Member

bradzacher commented Mar 8, 2019

maybe would be better if it was called no-enum-literal-comparison?

@mohsen1
Copy link
Contributor Author

mohsen1 commented Mar 8, 2019

It also prevents you from using literals instead enum params

@bradzacher
Copy link
Member

@mohsen1 - I can see a valid test for that, but there's no invalid test.

Correct me if I'm wrong - but I don't see you using the CallExpression selector, so it seems like that case isn't covered by this?

@mohsen1
Copy link
Contributor Author

mohsen1 commented Mar 8, 2019

oh wow! how did I miss that? I'll work on it. Meanwhile, let's finalize the naming.

@bradzacher
Copy link
Member

even though it doesn't technically cover the CallExpression case, I think that no-enum-literal-comparison still does a good job of explaining what the rule does.

@ThomasdenH
Copy link
Contributor

How about prefer-enum-member or use-enum-member?

@mysticatea
Copy link
Member

mysticatea commented Mar 11, 2019

This rule looks like to handle enums as a nominal type. (related: microsoft/TypeScript#202)

Because the type system of TS is structural typing, enums are compatible with the actual types (number or string) of each enum. Then, this rule tries to disable the compatibility of structural typing about enums. (in other words, this rule tries to make enums nominal typing.)

Now, how about nominal-typed-enum or no-structural-typed-enum?

EDIT: I may be misunderstanding something. 🤔

@bradzacher
Copy link
Member

my only problem with using nominal or structural in the rule name is that most TS users wouldn't know what those words mean.

I spend a decent chunk of time poking around typescript, and I still had to look up their meanings.

Maybe instead we could take a leaf out of flow's books and use the word opaque?
It's a more commonly used/understood word, so it would make it easier to understand the rule

opaque-enum / no-transparent-enum

@ThomasdenH
Copy link
Contributor

I like opaque-enum!

@mohsen1
Copy link
Contributor Author

mohsen1 commented Mar 12, 2019

I'm still working on this. Functions and methods with overloading and all that is complicated. I also like the name opaque-enum

@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Mar 15, 2019
@bradzacher bradzacher added the DO NOT MERGE PRs which should not be merged yet label Mar 21, 2019
@bradzacher bradzacher dismissed their stale review March 21, 2019 02:45

changes to come

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label May 9, 2019
@JamesHenry
Copy link
Member

Hi @mohsen1, thanks again for the PR, when do you think you will have chance to finalise this one?

@bradzacher bradzacher marked this pull request as draft April 13, 2020 01:04
@bradzacher bradzacher added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Apr 13, 2020
@bradzacher bradzacher closed this Apr 13, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party DO NOT MERGE PRs which should not be merged yet enhancement: new plugin rule New rule request for eslint-plugin help wanted Extra attention is needed stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: Disallow literals where enums are expected
6 participants