Added no-magic-numbers rule #1799

SimonSchick · 2016-11-29T00:13:32Z

PR checklist

Addresses an existing issue: #0000
New feature, bugfix, or enhancement
- Includes tests
Documentation update

What changes did you make?

Added a rule to prevent magic numbers.

See the added tests for examples.

Basic usage: "no-magic-numbers": true disallows all except 0 and 1/-1.
Custom allow list: "no-magic-numbers": [true, 1337, 1338]" Only 1337 and 1338 are allowed.

Is there anything you'd like reviewers to focus on?

I am not very familiar with tslint and the TS API and it would love to get feedback on the filter method.

palantirtech · 2016-11-29T00:13:32Z

Thanks for your interest in palantir/tslint, @SimonSchick! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

nchen63 · 2016-12-01T00:04:09Z

src/rules/noMagicNumbersRule.ts

+    /* tslint:disable:object-literal-sort-keys */
+    public static metadata: Lint.IRuleMetadata = {
+        ruleName: "no-magic-numbers",
+        description: "Disallows the use constant number values outside of variable assignment. -1, 0 and 1 are allowed by default.",


DEFAULT_ALLOWED does not include -1

It is as the minus sign is not part of number literal.

nchen63 · 2016-12-01T00:06:07Z

src/rules/noMagicNumbersRule.ts

+    /* tslint:disable:object-literal-sort-keys */
+    public static metadata: Lint.IRuleMetadata = {
+        ruleName: "no-magic-numbers",
+        description: "Disallows the use constant number values outside of variable assignment. -1, 0 and 1 are allowed by default.",


Description should say that the default values are set if there are no list of numbers is supplied. When I read this, it sounded like the default values are unioned with the user-supplied list

nchen63 · 2016-12-01T00:06:37Z

src/rules/noMagicNumbersRule.ts

+        rationale: Lint.Utils.dedent`
+            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 number.",


number -> numbers

nchen63 · 2016-12-01T00:08:45Z

src/rules/noMagicNumbersRule.ts

+            Rule.ALLOWED_NODES.indexOf(node.parent.kind) === -1) {
+            const options = this.getOptions();
+
+            const allowed: number[] = options.length > 0 ? options : Rule.DEFAULT_ALLOWED;


read options and do this logic in a constructor so you don't have to do this for every node

Ah, I wasn't entirely sure if this.getOptions might change during operation and would thus break things. Thanks for clearing that up.

nchen63 · 2016-12-01T00:13:54Z

src/rules/noMagicNumbersRule.ts

+
+    public static FAILURE_STRING = "'magic numbers' are no allowed";
+
+    public static ALLOWED_NODES = [


Use object literal with computed property names for faster lookups:

public static ALLOWED_NODES = { [ts.SyntaxKind.ExportAssignment]: true ... }

Rule.ALLOWED_NODES[node.parent.kind]

nchen63 · 2016-12-01T00:33:03Z

Looks pretty good in general. Have some minor changes

nchen63 · 2016-12-01T07:02:21Z

src/rules/noMagicNumbersRule.ts

+
+    public visitNode(node: ts.Node) {
+        if (node.kind === ts.SyntaxKind.NumericLiteral && !Rule.ALLOWED_NODES[node.parent.kind]) {
+            if (!this.allowed[node.getText()]) {


this should be changed to handle negative numbers. If someone configures the rule to allow -1, it would never be matched.

I'll have to look into that, I'm a little short on time right now tho...

nchen63 · 2016-12-01T07:03:29Z

test/rules/no-magic-numbers/default/test.ts.lint

@@ -0,0 +1,30 @@
+console.log(-1, 0, 1);


add test for floats

adidahiya · 2016-12-08T18:13:16Z

src/rules/noMagicNumbersRule.ts

+        [ts.SyntaxKind.PropertyDeclaration]: true,
+    };
+
+    public static DEFAULT_ALLOWED = [


minor: can you just make this a one liner? i.e. ... = [0, 1];

adidahiya · 2016-12-08T18:14:31Z

src/rules/noMagicNumbersRule.ts

+        super(sourceFile, options);
+
+        const configOptions = this.getOptions();
+        const allowedArray: number[] = configOptions.length > 0 ? configOptions : Rule.DEFAULT_ALLOWED;


please be more specific in the variable name and omit "Array" since it is redundant with the typdef. call this const allowedNumbers instead.

adidahiya · 2016-12-08T18:15:15Z

src/rules/noMagicNumbersRule.ts

+}
+
+class NoMagicNumbersWalker extends Lint.RuleWalker {
+    private allowed: { [prop: string]: boolean };


what is the point of this object? its values are always true.

#1799 (comment) it's used as a lookup map (which is faster).

maybe add a comment so people know that this is for fast lookup

SimonSchick · 2016-12-08T18:42:21Z

I think I addressed all comments for now.

comments addressed

nchen63 · 2016-12-08T19:13:50Z

src/rules/noMagicNumbersRule.ts

+        ruleName: "no-magic-numbers",
+        description: Lint.Utils.dedent`
+            Disallows the use constant number values outside of variable assignments.
+            When no list of allowed values is specified, 0 and 1 are allowed by default.`,


this doesn't match DEFAULT_ALLOWED

Not anymore, right 😛

nchen63 · 2016-12-08T19:14:09Z

src/rules/noMagicNumbersRule.ts

+    };
+    /* tslint:enable:object-literal-sort-keys */
+
+    public static FAILURE_STRING = "'magic numbers' are no allowed";


no -> not

nchen63 · 2016-12-08T19:15:57Z

src/rules/noMagicNumbersRule.ts

+}
+
+class NoMagicNumbersWalker extends Lint.RuleWalker {
+    private allowed: { [prop: string]: boolean };


maybe add a comment so people know that this is for fast lookup

nchen63 · 2016-12-08T20:47:33Z

@SimonSchick looks good. Thanks!

Added no-magic-numbers rule

814e180

include enums and static members

801f9ab

nchen63 reviewed Dec 1, 2016

View reviewed changes

clarify functionality and optimisations

3d3896b

nchen63 suggested changes Dec 1, 2016

View reviewed changes

nchen63 added the PR: waiting for author label Dec 7, 2016

added float tests

92ffece

adidahiya previously requested changes Dec 8, 2016

View reviewed changes

SimonSchick added 2 commits December 8, 2016 19:39

added support for negative magic numbers

11ca88a

comments

ac5e220

nchen63 reviewed Dec 8, 2016

View reviewed changes

fixed message, added more comments and removed some redundant code

2e353e7

nchen63 approved these changes Dec 8, 2016

View reviewed changes

nchen63 merged commit 8e6a0ce into palantir:master Dec 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added no-magic-numbers rule #1799

Added no-magic-numbers rule #1799

SimonSchick commented Nov 29, 2016 •

edited

Loading

palantirtech commented Nov 29, 2016

nchen63 Dec 1, 2016

SimonSchick Dec 1, 2016

nchen63 Dec 1, 2016

nchen63 Dec 1, 2016

nchen63 Dec 1, 2016

SimonSchick Dec 1, 2016

nchen63 Dec 1, 2016

nchen63 commented Dec 1, 2016

nchen63 Dec 1, 2016

SimonSchick Dec 3, 2016

nchen63 Dec 1, 2016

adidahiya Dec 8, 2016

adidahiya Dec 8, 2016

adidahiya Dec 8, 2016

SimonSchick Dec 8, 2016 •

edited

Loading

nchen63 Dec 8, 2016 •

edited

Loading

SimonSchick commented Dec 8, 2016

nchen63 Dec 8, 2016

SimonSchick Dec 8, 2016

nchen63 Dec 8, 2016

nchen63 Dec 8, 2016 •

edited

Loading

nchen63 commented Dec 8, 2016


		public static FAILURE_STRING = "'magic numbers' are no allowed";

		public static ALLOWED_NODES = [

Added no-magic-numbers rule #1799

Added no-magic-numbers rule #1799

Conversation

SimonSchick commented Nov 29, 2016 • edited Loading

PR checklist

What changes did you make?

Is there anything you'd like reviewers to focus on?

palantirtech commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchen63 commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonSchick Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

nchen63 Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

SimonSchick commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nchen63 Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

nchen63 commented Dec 8, 2016

SimonSchick commented Nov 29, 2016 •

edited

Loading

SimonSchick Dec 8, 2016 •

edited

Loading

nchen63 Dec 8, 2016 •

edited

Loading

nchen63 Dec 8, 2016 •

edited

Loading