New Magic Numbers rule is not usable in practice

[Martin-Luft]

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

[BleedingDev]

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: typescript-eslint/typescript-eslint#934, typescript-eslint/typescript-eslint#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! 👋