Added a no-this-reassignment rule #2931
Conversation
Bans phrases like "var self = this;". Port of the [tslint-microsoft-contrib no-var-self rule](https://github.com/Microsoft/tslint-microsoft-contrib/blob/master/src/noVarSelfRule.ts). This commit does not yet add it to tslint:all.
No violations in the code base
src/rules/noSelfThisRule.ts
Outdated
| [ | ||
| true, | ||
| { | ||
| [ALLOWED_THIS_NAMES]: ["self"], |
There was a problem hiding this comment.
since this is a regex, the value should be ^self$
src/rules/noSelfThisRule.ts
Outdated
| && node.name.kind === ts.SyntaxKind.Identifier | ||
| && this.variableNameIsBanned(node.name.text) | ||
| ) { | ||
| this.addFailureAt(node.getStart(), node.getWidth(), Rule.FAILURE_STRING_FACTORY(node.name.text)); |
Will do.
Seems reasonable. I'll add it as an on-by-default option. |
JoshuaKGoldberg
changed the title
Also fixed tests to be under the right name...
src/rules/noThisReassignmentRule.ts
Outdated
|
|
||
| import * as Lint from "../index"; | ||
|
|
||
| const ALLOW_THIS_DESTRUCTURING = "allow-this-destructuring"; |
There was a problem hiding this comment.
simply "allow-destructuring"?
src/rules/noThisReassignmentRule.ts
Outdated
| import * as Lint from "../index"; | ||
|
|
||
| const ALLOW_THIS_DESTRUCTURING = "allow-this-destructuring"; | ||
| const ALLOWED_THIS_NAMES = "allowed-this-names"; |
src/rules/noThisReassignmentRule.ts
Outdated
| * \`${ALLOWED_THIS_NAMES}\` may be specified as a list of regular expressions to match allowed variable names.`, | ||
| rationale: "Assigning a variable to `this` instead of properly using arrow lambdas" | ||
| + "may be a symptom of pre-ES6 practices or not manging scope well.", | ||
| ruleName: "no-this-reassignment", |
There was a problem hiding this comment.
I'd prefer no-this-assign(ment), because you cannot reassign this anyway
src/rules/noThisReassignmentRule.ts
Outdated
| typescriptOnly: false, | ||
| }; | ||
|
|
||
| public static readonly FAILURE_STRING_BINDINGS = "Don't reassign members of `this` to local variables."; |
src/rules/noThisReassignmentRule.ts
Outdated
| const allowedThisNames: string[] = []; | ||
| let allowThisDestructuring = false; | ||
|
|
||
| for (const ruleArgument of this.ruleArguments as RuleArgument[]) { |
There was a problem hiding this comment.
why the loop? there should only be one object in the config
src/rules/noThisReassignmentRule.ts
Outdated
| [ | ||
| true, | ||
| { | ||
| [ALLOWED_THIS_NAMES]: ["self"], |
There was a problem hiding this comment.
I'd still prefer "^self$" here to show that this is a regular expression
src/rules/noThisReassignmentRule.ts
Outdated
| } | ||
| break; | ||
|
|
||
| case ts.SyntaxKind.ObjectBindingPattern: |
There was a problem hiding this comment.
make this the default clause. that will also handle array destructuring
src/configs/all.ts
Outdated
| @@ -51,6 +51,7 @@ export const rules = { | |||
| "no-namespace": true, | |||
| "no-non-null-assertion": true, | |||
| "no-reference": true, | |||
| "no-this-reassignment": true, | |||
There was a problem hiding this comment.
also add this to tslint:latest
Also not using a for loop for options and added to :latest.
|
Oh, almost forgot! This was made with help from @sethbrenith; he should be mentioned in any contributors list. |
|
Thanks for the shout-out, but I think this is all you. The only thing I wrote is a fixer for it, which doesn't seem to be included here. |
PR checklist
Overview of change:
Bans phrases like "var self = this;". Port of the tslint-microsoft-contrib no-var-self rule.
CHANGELOG.md entry:
[new-rule]
no-this-reassignment