Tslint: New Magic Numbers rule is not usable in practice

[Martin-Wegner]

Bug Report

• __TSLint version__: 4.1.0
• __TypeScript version__: 2.0.10
• __Running TSLint via__: Visual Studio / WebPack 2

Example: a 5 minute read timeout

private static READ_TIMEOUT_IN_MILLIS: number = 5 * 60 * 1000; // ERROR

private static SECOND_IN_MILLIS: number = 1000;
private static MINUTE_IN_MILLIS: number = 60 * X.SECOND_IN_MILLIS; // ERROR
private static READ_TIMEOUT_IN_MILLIS: number = 5 * X.MINUTE_IN_MILLIS; // ERROR

The only thing that works at the moment:

private static READ_TIMEOUT_IN_MILLIS: number = 300000; // is that readable???

[adidahiya]

That is a fair assessment. I think the rule should be more lenient. Tagging @SimonSchick

[SimonSchick]

Sorry I missed that specific use case, I'll look into it tomorrow.

[nchen63]

I personally think it's fine the way it is now. You'd just need to define more things, but I think that's in the spirit of this rule.

private static SECOND_IN_MILLIS: number = 1000;
private static MINUTE_IN_SECONDS = 60;
private static MINUTE_IN_MILLIS: number = X.MINUTE_IN_SECONDS * X.SECOND_IN_MILLIS;
private static TIMEOUT_IN_MINUTES = 5;
private static READ_TIMEOUT_IN_MILLIS: number = X.TIMEOUT_IN_MINUTES * X.MINUTE_IN_MILLIS;

[SimonSchick]

That's rather cumbersome, some flexibility would be nice.

[chrismbarr]

This rule in general seems overly strict, which is why I've just decided to disable the rule. I say just disable the rule if it's something that doesn't work well for your team or workflow. All the rules are optional.

[RevanProdigalKnight]

An idea for making this more flexible could be to allow magic numbers if there's a block comment (with content) explaining what the magic number is directly following the number, like what TypeScript already does when compiling values from a const enum into JavaScript, e.g.:

const MILLIS_IN_SECOND = 1000 /* milliseconds in second */; // Allowed

let secondsInMinute: number = 60 * MILLIS_IN_SECOND; // Not allowed
let secondsInMinute: number = 60 /**/ * MILLIS_IN_SECOND; // Not allowed

let time: number = 60 /* seconds in minute */ * MILLIS_IN_SECOND; // Allowed

let idx: number = 'Slashes are awesome // !'.lastIndexOf('/') - 2; // Not allowed
let idx: number = 'Slashes are awesome // !'.lastIndexOf('/') - 2 /* get character before / */; // Allowed

[nfantone]

@adidahiya Another fair approach would be to allow relatively "common", low numbers. Say, integers in a '[-10, 100] range. ~Or even better: make allowed numbers configurable from tslint.json~ (just realised that this, though cumbersome, is _already_ possible).

This would make situations like the one below to not be problem for the linter.

 expect(doAction).toHaveBeenCalledTimes(3);
// [tslint] 'magic numbers' are not allowed (no-magic-numbers)

My common sense tells me that 3 in that context is completely self-explanatory and pretty non-magical. I would not expect any fellow developer to go:

const numberOfTimesDoActionShouldHaveBeenCalled = 3;
expect(doAction).toHaveBeenCalledTimes(numberOfTimesDoActionShouldHaveBeenCalled);

However, this:

 expect(doAction).toHaveBeenCalledTimes(1398);

_Should_ raise some eyebrows.

[vmasek]

Another example is when trying to have constants that hold values for a pager.

const pageSizeOptions = [10, 50, 100];

It would be a nice feature if rule would accept 'ignore-constants' option that should turn it off for const and maybe also readonly variables

[aervin]

The PR I opened would allow this

const pageSizeOptions = [10, 50, 100]; /* any comment at all */

[vmasek]

What is the status of this issue? Are the PRs being merged? Example with pageSizeOptions above is still handled as error

[pegak]

Milliseconds are still problem, could this be fixed? I think that time constants are used a lot. :)

[Rolandisimo]

I believe battling a tslint rule severity by adding a code smell is counterproductive and desperate. The rule is driving me nuts sometimes too.

Currently there are 2 options for this rule in tslint e.g. { allowed-numbers": [1, 2, 3], "ignore-jsx": true} and a severity mode. This is not satisfying. Thus, I am looking into creating a custom tslint rule and disabling the original one. Until then, I'm adding //tslint-disable:next-line no-magic-numbers for cases like toHaveBeenCalledTimes in tests. 😧 Which I know kinda contradicts my first sentence. 😵🔫

[superdyzio]

@aervin is there any param to allow cases like you mentioned?

[adidahiya]

Closing in favor of eslint rule: https://github.com/typescript-eslint/typescript-eslint/issues/934, https://github.com/typescript-eslint/typescript-eslint/pull/938

[JoshuaKGoldberg]

I seem to recall this image being different... 😄

[JoshuaKGoldberg]

🤖 Beep boop! 👉 TSLint is deprecated 👈 _(#4534)_ and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋