feat(eslint-plugin): [no-magic-numbers] add ignoreReadonlyClassProperties option

[a-tarasyuk]

Fixes #934

[typescript-eslint]

Thanks for the PR, @a-tarasyuk!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint

[bradzacher]

Thanks for doing this!
Code LGTM.
Could you please add this to the rule readme as well?

[a-tarasyuk]

@bradzacher updated docs.

[bradzacher]

LGTM - thanks for doing this!

[glen-84]

Hmm.

Shouldn't this be allowed by default? In fact, shouldn't any assignment of a number to a named variable be allowed? Isn't the definition of a magic number a number that is "unnamed"?

For example:

class Example {
    // Not magic.
    public static readonly SECONDS = 60;
    // Magic ... what is one of the 60s referring to?
    public static readonly MINUTES = 60 * 60;
}

(it doesn't even need to be static or readonly)

... confirmed with TSLint:

This rule (at least TSLint's version), is already quite difficult to work with, so we shouldn't make it more difficult.

[phaux]

I think it shouldn't even need an option. readonly class props should be simply handled the same as const declarations

[bradzacher]

I think an option makes sense.

Not everyone might want to allow readonly props as magic number holders.
The rule has been live for a number of months, and #934 is the first time it's been asked, so that would suggest this is the case.

Having it off by default is partially for compatibility, otherwise it relaxes the rule's default checks - which is a breaking change.

[glen-84]

So basically, by default:

var x = 5;

^ This is valid.

readonly x = 5;

^ But this is not?

That doesn't really make sense to me.

From the TSLint description: "Disallows the use [of] constant number values outside of variable assignments."

That's all it's supposed to do.

The rule has been live for a number of months, and #934 is the first time it's been asked.

2nd time. I was going to open the same issue, but decided against it because the rule is (in general) quite difficult to use, especially on an existing codebase.

Also keep in mind that it is not included in the recommended configurations, and I'm pretty sure that it's one of the lesser-used rules.

[phaux]

In the end it doesn't matter that much, because you can always replace a readonly class prop with a const. Having number constant as instance member is unnecessary overhead.

[bradzacher]

all of the options added by this plugin are currently turned off by default.
I don't use this rule because I don't agree with it personally, and nobody asked beforehand, so it wasn't on my radar to turn the options on to default for the v2 release.

Can certainly consider turning them on for v3, but I don't see the point of holding back this PR until we are ready for another major bump.

If you feel strongly that the options should be on by default, happy for you to raise an issue so we can track it, and get it in with the v3 release.

[glen-84]

Having number constant as instance member is unnecessary overhead.

It would be static.

If you feel strongly that the options should be on by default, happy for you to raise an issue so we can track it, and get it in with the v3 release.

Well it seems that I'm the only one with this opinion, so I'll just leave it then.

[JamesHenry]

I’d be on board with changing the default of the option in v3, I think readonly properties clearly satisfy the same idea as consts when it comes to identifying otherwise “magic numbers”, but agree it would be a breaking change to do it now

[JamesHenry]

Thanks!

[codecov]

Codecov Report

Merging #938 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #938      +/-   ##
==========================================
+ Coverage   94.13%   94.13%   +<.01%     
==========================================
  Files         115      115              
  Lines        5012     5017       +5     
  Branches     1400     1403       +3     
==========================================
+ Hits         4718     4723       +5     
  Misses        166      166              
  Partials      128      128

Impacted Files	Coverage Δ
...ckages/eslint-plugin/src/rules/no-magic-numbers.ts	92.5% <100%> (+1.07%)	⬆️