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

Rule Change: Should no-magic-numbers be moved out of core #18439

Closed
1 task
Samuel-Therrien-Beslogic opened this issue May 10, 2024 · 2 comments
Closed
1 task
Labels
enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Comments

@Samuel-Therrien-Beslogic
Copy link

Samuel-Therrien-Beslogic commented May 10, 2024

What rule do you want to change?

no-magic-numbers

What change do you want to make?

Implement suggestions

How do you think the change should be implemented?

Other

Example code

// Fail

const DEFAULT_PAGE_LENGTH = [5, 10, 25, 100]

class MyCLass {
  readonly DEFAULT_PAGE_LENGTH = [5, 10, 25, 100]
}

let DEFAULT_PAGE_LENGTH = [5, 10, 25, 100] as const

class MyCLass {
  DEFAULT_PAGE_LENGTH = [5, 10, 25, 100] as const
}

// Pass

const DEFAULT_PAGE_LENGTH = [5, 10, 25, 100] as const

class MyCLass {
  readonly DEFAULT_PAGE_LENGTH = [5, 10, 25, 100] as const
}

What does the rule currently do for this code?

Above is a request I have for this rule under TypeScript to support readonly arrays.
This rule is frozen by both the ESLint and TypeScript-ESLint team for being stylistic, which is fair. typescript-eslint/typescript-eslint#8102 (comment)

However, ESLint-stylistic, which would be the next in line logically to take on this rule, don't want to because it's a in core and not deprecated in ts-eslint eslint-stylistic/eslint-stylistic#238

But ts-eslint don't want to deprecate it either because it's not deprecated in core eslint typescript-eslint/typescript-eslint#8790

So everyone's just passing the ball around due to project policies. And this gets stuck in a limbo.

What will the rule do after it's changed?

I am henceforth requesting for this rule to be deprecated as a stylistic rule. Or for relevant teams to try to come on an agreement on who's to be responsible for maintaining this rule. CCing relevant parties to this chain of discussion: @Martinspire @bradzacher @antfu @nzakas

And just to catch a certain comment ahead of time, I don't think that "re-implementing it myself in a new plugin" is really the way to go here, even if I plan on eventually learning about making custom AST rules w/o no-restricted-syntax. However, if there are other well-known community plugins that may be interested and support typescript, please let me know, I'll suggest it there.

Participation

  • I am willing to submit a pull request to implement this change.

Additional comments

Transfering and expanding this request from #17681 (comment)
This rule is stuck in limbo because it's "Stylistic, but still in core, so no one touches it".

@Samuel-Therrien-Beslogic Samuel-Therrien-Beslogic added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules labels May 10, 2024
@bradzacher
Copy link
Contributor

on who's to be responsible for maintaining this rule

I thought that I made this pretty clear in this comment typescript-eslint/typescript-eslint#8790 (comment)

Being feature frozen and unmaintained are very, very different things.
We will continue to enhance it with new TS features that align with the existing style and we will fix bugs as reported. We will also keep it in sync with the base rule as it changes.

We are just not accepting any new features.

The rule is not unmaintained by either eslint core or typescript eslint. Both are fully maintained - we both have frozen features in the stylistic rule.

You can read more about why we have both frozen the rule here: https://eslint.org/blog/2020/05/changes-to-rules-policies/


The fact that eslint-stylistic doesn't want to take on a non-deprecated rule is on them and their policies - though I find it a perfectly reasonable choice! They want to be the home of rules that are being removed from core/typescript-eslint, not the home for rules that are frozen.
That sounds like a pretty clear and sound policy to me.


So everyone's just passing the ball around due to project policies.

I don't agree with that statement at all. Two projects are saying that it's fully supported and maintained but not getting new features and a third is saying they don't want to take it on because it's fully supported and maintained.

There's no limbo or ball passing - it's in a pretty clearly defined state of "maintained and frozen".

I don't think that "re-implementing it myself in a new plugin" is really the way to go here

But why not? If you disagree with the policy of 3 separate projects who say that the state is valid and not changing - then really the option is to either accept it or fork it.

@nzakas
Copy link
Member

nzakas commented May 13, 2024

But why not? If you disagree with the policy of 3 separate projects who say that the state is valid and not changing - then really the option is to either accept it or fork it.

I think this is well-said by @bradzacher. I don't see a reason to make a change at this point, but if you'd like, you can certainly fork the rule to do exactly what you want. That's the way ESLint was designed!

@nzakas nzakas closed this as not planned Won't fix, can't repro, duplicate, stale May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

3 participants