-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[member-ordering] Add ability to sort enums #4859
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
Comments
Hmm, I like this idea, but it's a little tricky. Some projects are intentional about the order of their enums: enum Direction {
None, // 0
Horizontal, // 1
Vertical, // 2
Both // 3
} We could make sorting enums opt-in. It shouldn't flag enum values with initializers ( Tangentially, I'm finding the https://typescript-eslint.io/rules/member-ordering docs a bit confusing. Filed #4861 to clarify them. In the meantime, eslint-plugin-typescript-sort-keys has a string-enum rule that does [edit: something similar to] what you're looking for. |
Hey Josh, thanks for the quick reply.
I need to sort numeric enums, like the |
Ah, sorry, you're right - that's missing in the plugin. 👎 |
Yeah I do understand the though process here - but it can be really dangerous to do this for implicitly valued enums because TS does not restrict comparisons against enums. This means in your code you might have a comparison between the enum and a number (which TS fully understands), and by sorting the enum you would break your code. enum Foo {
A,
B,
}
function test(arg: Foo) {
if (arg === 1) {
arg
// ^? (parameter) arg: Foo.B
}
} This might be really difficult to debug and understand as it would break things at a distance, and potentially without any type errors. I think if we do this, we should only implement it for enums with explicit values. |
Is there a common use case for comparisons like If we add a strong caveat to the docs, I would think it preferable that users take on the risk of fixing on their own. If the rule ever receives a suggestion autofix, this would be a case where we wouldn't want to emit one. |
TS doesn't block it, and even understands it - meaning it's 100% on the dev to make sure the code doesn't sneak into the codebase. There is a whole thing around number-typed enums and flags and how damn unsafe they are.. There is also some degree of "yeah it kind of makes sense" for them because you might do like There's also a whole discussion about public APIs. If you export an enum and someone consumes your library from raw JS - technically they're well within their rights to pass a number instead of explicitly using the enum. So by reordering the enum you would introduce a breaking change in your API.
We also have the |
#308 is probably the best proposition for a linting rule that I have ever seen. Once the rule exists, it seems like it should be immediately turned on for virtually any TypeScript codebase. The Thus, in a world where the rule from #308 does exist and is included in the Should I try to submit a PR for #308 first? It seems like it would be fairly easy. |
It's useful for the above reasons I mentioned - to ensure the public API is clear and consistent. Implicit initialisers can be pretty unsafe and dangerous unless their use is very, very targeted and limited. So the lint rule does have use to enforce a better standard in your codebase!
You can certainly try if you'd like! It's not as easy as you might think just because you will need to cover all of the "comparison" locations, all of the "assignment" locations, and also correctly gather the types from those locations. It's not super trivial to do this!!! |
Ok. I understand the problem now. Authors of TypeScript libraries need to consider JavaScript consumers, so it is safer to not have the warning turned on by default, since it could cause people to accidentally break their API. I'm not sure how to restructure the PR to accomplish that though, so I think I'll need a little help with that part. |
If anyone needs enum sorting, I'm hosting a modified version of the rule here: https://github.com/IsaacScript/isaacscript/blob/main/packages/eslint-plugin-isaacscript/docs/rules/member-ordering.md (You can consume it from NPM via the All it does is implement this PR: #4862 |
I think this is same case as another feature request for sorting enums, #4008 (comment) . There is another eslint plugin that you can use if needed ( https://github.com/azat-io/eslint-plugin-perfectionist ) |
|
Uh oh!
There was an error while loading. Please reload this page.
Hello, and thanks for your excellent linter.
Currently, the
@typescript-eslint/member-ordering
rule supports sorting classes, interfaces, and type literals.But it doesn't support enums. Can someone please add enums? It seems like it would be a super-quick addition.
For example, with the following ESLint configuration:
ESLint should warn me that the following enum is not in alphabetical order:
The text was updated successfully, but these errors were encountered: