[no-magic-numbers] option "allow-element-access" and ignore nums w/ comments #3605
Conversation
… line as a comment
| }, | ||
| optionExamples: [true, [true, 1, 2, 3]], | ||
| optionExamples: [true, [true, 1, 2, 3], [true, { |
There was a problem hiding this comment.
this second example no longer matches the schema, which makes this PR a breaking change
There was a problem hiding this comment.
though i see you've preserved support for the old usage below.
| @@ -62,15 +89,55 @@ export class Rule extends Lint.Rules.AbstractRule { | |||
|
|
|||
| public static DEFAULT_ALLOWED = [ -1, 0, 1 ]; | |||
|
|
|||
| /* tslint:disable no-unsafe-any */ | |||
There was a problem hiding this comment.
wait why... can we not disable this rule here?
| ); | ||
| } | ||
|
|
||
| private parseOptions(args: any): Options { |
There was a problem hiding this comment.
from code, looks like this is args?: number[] | { [key: string]: boolean }
| import * as Lint from "../index"; | ||
| import { isNegativeNumberLiteral } from "../language/utils"; | ||
|
|
||
| interface Options { | ||
| allowedNumbers: number[] | Set<string>; |
There was a problem hiding this comment.
Set<string> does not make sense for numbers. why is that here and can it be just number[] (or Set<number>)?
| public walk(sourceFile: ts.SourceFile) { | ||
| const cb = (node: ts.Node): void => { | ||
| if ( | ||
| commentPositions(this.sourceFile) |
There was a problem hiding this comment.
❌ strongly against this feature. instead of implicitly disabling the rule if a comment appears, users should explicitly use the existing // tslint:disable-next-line feature, which is always a comment itself!
There was a problem hiding this comment.
I see a big difference here:
foo = bar[1]; // some helpful info about "1"and
// tslint:disable-next-line no-magic-numbers
foo = bar[1];Please keep in mind that this PR is in response to issues about the strictness of no-magic-numbers.
| * Used for checking when a magic number is on the same line as | ||
| * a comment. In such a case, the magic number should be ignored. | ||
| */ | ||
| function commentPositions(sourceFile: ts.SourceFile): number[] { |
| !( | ||
| node.parent !== undefined && | ||
| isElementAccessExpression(node.parent) && | ||
| this.options.allowElementAccess |
There was a problem hiding this comment.
please pull this () out to a local variable so it's easier to follow this logic.
| @@ -28,3 +34,18 @@ class A { | |||
| enum Test { | |||
| key = 1337, | |||
| } | |||
|
|
|||
| it('should provide a link to the landing page', () => { | |||
There was a problem hiding this comment.
while it's cool to put real code here, it's also very confusing when the only thing we care about is that little index operator inside the next line.
please remove the it() wrapper and just test the array access.
|
@giladgray We can nix this PR if you like. To me, the main benefit here is the ability to add inline comments so this rule ignores the magic number. Devs may use a |
|
ok cool will close. thanks for the info @aervin! fwiw, you can totally add a comment after the disable: |
|
Alright thanks @giladgray |
PR checklist
Overview of change:
allow-element-accessallowed-numbersconfig option into config objectIs there anything you'd like reviewers to focus on?
The way this rule gathers the source file's comments--is it sufficient? Are there edge cases I'm forgetting? See: the
generateCommentPositionsmethod.