Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

add jsx-ignore option to no-magic-numbers rule #4460

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
88 changes: 82 additions & 6 deletions src/rules/noMagicNumbersRule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,21 @@ import { isCallExpression, isIdentifier } from "tsutils";
import * as Lint from "../index";
import { isNegativeNumberLiteral } from "../language/utils";

interface AllowedNumbersType {
"allowed-numbers": number[];
}
interface IgnoreJsxType {
"ignore-jsx": boolean;
}
interface ParsedOptionsType {
"allowed-numbers": number[];
"ignore-jsx"?: boolean;
}
type OptionsType = IgnoreJsxType | AllowedNumbersType;

const IGNORE_JSX_OPTION = "ignore-jsx";
const ALLOWED_NUMBERS_OPTION = "allowed-numbers";

export class Rule extends Lint.Rules.AbstractRule {
/* tslint:disable:object-literal-sort-keys */
public static metadata: Lint.IRuleMetadata = {
Expand All @@ -32,15 +47,37 @@ export class Rule extends Lint.Rules.AbstractRule {
Magic numbers should be avoided as they often lack documentation.
Forcing them to be stored in variables gives them implicit documentation.
`,
optionsDescription: "A list of allowed numbers.",
optionsDescription: Lint.Utils.dedent`
Options may either be a list of numbers to ignore (not consider 'magic'), or an object containing up to two properties:
* \`${ALLOWED_NUMBERS_OPTION}\` as the list of numbers to ignore.
* \`${IGNORE_JSX_OPTION}\` to specify that 'magic' numbers should be allowed as JSX attributes.`,
options: {
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
type: "array",
items: {
type: "number",
},
properties: {
type: "object",
[ALLOWED_NUMBERS_OPTION]: {
type: "array",
},
[IGNORE_JSX_OPTION]: {
type: "boolean",
},
},
minLength: 1,
},
optionExamples: [true, [true, 1, 2, 3]],

optionExamples: [
[true, 1, 2, 3],
[
true,
{
[ALLOWED_NUMBERS_OPTION]: [1, 2, 3],
[IGNORE_JSX_OPTION]: true,
},
],
],
type: "typescript",
typescriptOnly: false,
};
Expand Down Expand Up @@ -70,13 +107,37 @@ export class Rule extends Lint.Rules.AbstractRule {
new NoMagicNumbersWalker(
sourceFile,
this.ruleName,
this.ruleArguments.length > 0 ? this.ruleArguments : Rule.DEFAULT_ALLOWED,
this.ruleArguments.length > 0
? parseOptions(this.ruleArguments)
: parseOptions(Rule.DEFAULT_ALLOWED),
),
);
}
}

class NoMagicNumbersWalker extends Lint.AbstractWalker<number[]> {
function parseOptions(options: Array<OptionsType | number>): ParsedOptionsType {
const parsedOptions: ParsedOptionsType = { [ALLOWED_NUMBERS_OPTION]: [] };
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
for (const option of options) {
if (typeof option === "number") {
parsedOptions[ALLOWED_NUMBERS_OPTION].push(option);
continue;
}
if (option.hasOwnProperty(ALLOWED_NUMBERS_OPTION)) {
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
const numberOptions = (option as AllowedNumbersType)[ALLOWED_NUMBERS_OPTION];
if (Array.isArray(numberOptions) && numberOptions.length > 0) {
numberOptions.forEach((num: number) =>
parsedOptions[ALLOWED_NUMBERS_OPTION].push(num),
);
}
}
if (option.hasOwnProperty(IGNORE_JSX_OPTION)) {
parsedOptions[IGNORE_JSX_OPTION] = (option as IgnoreJsxType)[IGNORE_JSX_OPTION];
}
}
return parsedOptions;
}

class NoMagicNumbersWalker extends Lint.AbstractWalker<ParsedOptionsType | number[]> {
public walk(sourceFile: ts.SourceFile) {
const cb = (node: ts.Node): void => {
if (
Expand All @@ -101,11 +162,26 @@ class NoMagicNumbersWalker extends Lint.AbstractWalker<number[]> {
return ts.forEachChild(sourceFile, cb);
}

private isAllowedNumber(num: string): boolean {
const numberOptions = (this.options as ParsedOptionsType)[ALLOWED_NUMBERS_OPTION];
if (numberOptions.length > 0) {
return numberOptions.some((allowedNum: number) =>
/* Using Object.is() to differentiate between pos/neg zero */
Object.is(allowedNum, parseFloat(num)),
);
}
return false;
}

private checkNumericLiteral(node: ts.Node, num: string) {
/* Using Object.is() to differentiate between pos/neg zero */
const shouldIgnoreJsxExpression =
node.parent.kind === ts.SyntaxKind.JsxExpression &&
(this.options as ParsedOptionsType)[IGNORE_JSX_OPTION];

if (
!Rule.ALLOWED_NODES.has(node.parent.kind) &&
!this.options.some(allowedNum => Object.is(allowedNum, parseFloat(num)))
!this.isAllowedNumber(num) &&
!shouldIgnoreJsxExpression
) {
this.addFailureAtNode(node, Rule.FAILURE_STRING(num));
}
Expand Down
23 changes: 23 additions & 0 deletions test/rules/no-magic-numbers/allowed-numbers/test.ts.lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
parseInt();
parseInt('123', 2);
parseInt('123', 8);
parseInt('123', 10);
parseInt('123', 16);
console.log(-0);
console.log(1337);
console.log(-1337);
console.log(- 1337);
console.log(1337.7);
console.log(1338);
~~~~ ['magic numbers' are not allowed: 1338]
console.log(-1338)
~~~~~ ['magic numbers' are not allowed: -1338]
parseInt(foo === 4711 ? bar : baz, 10);
~~~~ ['magic numbers' are not allowed: 4711]
parseInt(foo === -0 ? bar : baz, 10);
export let x = 1337;
export let x = -1337;
export let x = 1337.7;
export let x = 1338;
export let x = -1338;
export let x = -0;
7 changes: 7 additions & 0 deletions test/rules/no-magic-numbers/allowed-numbers/tslint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"rules": {
"no-magic-numbers": [true, {
"allowed-numbers": [1337, 1337.7, -1337, -0]
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
}]
}
}
8 changes: 8 additions & 0 deletions test/rules/no-magic-numbers/default-jsx/test.tsx.lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import * as React from "react";
class App extends React.Component {
render() {
return <Component width={200} height={200} />;
~~~ ['magic numbers' are not allowed: 200]
~~~ ['magic numbers' are not allowed: 200]
}
}
5 changes: 5 additions & 0 deletions test/rules/no-magic-numbers/default-jsx/tslint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"rules": {
"no-magic-numbers": true
}
}
10 changes: 10 additions & 0 deletions test/rules/no-magic-numbers/ignore-jsx/test.tsx.lint
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import * as React from "react";
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
class App extends React.Component {
render() {
return <Component width={200} height={200} />;
}
}

function something(num) {}
something(100)
~~~ ['magic numbers' are not allowed: 100]
7 changes: 7 additions & 0 deletions test/rules/no-magic-numbers/ignore-jsx/tslint.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"rules": {
"no-magic-numbers": [true, {
"ignore-jsx": true
}]
}
}