Refactor variable-name rule #2355
Conversation
src/rules/variableNameRule.ts
Outdated
| import * as ts from "typescript"; | ||
|
|
||
| import * as Lint from "../index"; | ||
|
|
||
| const BANNED_KEYWORDS = ["any", "Number", "number", "String", "string", "Boolean", "boolean", "Undefined", "undefined"]; | ||
| const BANNED_KEYWORDS = new Set(["any", "Number", "number", "String", "string", "Boolean", "boolean", "Undefined", "undefined"]); | ||
| const bannedKeywordsStr = Array.from(BANNED_KEYWORDS).map((kw) => `\`${kw}\``).join(", "); |
There was a problem hiding this comment.
why initialize a Set with an array and the convert it back to an array? Wouldn't it make more sense to just use the same array for both?
Also IIRC a set does not guarantee any order, which could separate Number and number in the output for example.
src/rules/variableNameRule.ts
Outdated
| function isCamelCase(name: string, options: Options): boolean { | ||
| const firstCharacter = name.charAt(0); | ||
| const lastCharacter = name.charAt(name.length - 1); | ||
| const middle = name.substr(1, name.length - 2); |
There was a problem hiding this comment.
could be simplified to name.slice(1, -1)
src/rules/variableNameRule.ts
Outdated
| const lastCharacter = name.charAt(name.length - 1); | ||
| const middle = name.substr(1, name.length - 2); | ||
| function isCamelCase(name: string, options: Options): boolean { | ||
| const firstCharacter = name.charAt(0); |
There was a problem hiding this comment.
str.chatAt(pos) vs str[pos] is your personal preference?
I prefer the latter, because it is shorter. But that's totally nice to have.
src/rules/variableNameRule.ts
Outdated
| return false; | ||
| } | ||
| return middle.indexOf("_") === -1; | ||
| if (name.length <= 0) { |
There was a problem hiding this comment.
I know that check was there before, but when would this ever be true?
Edit: function declarations without a name have an identifier with zero width, but that is invalid syntax anyway.
src/rules/variableNameRule.ts
Outdated
|
|
||
| public static FORMAT_FAILURE = "variable name must be in camelcase or uppercase"; | ||
| public static KEYWORD_FAILURE = "variable name clashes with keyword/type"; | ||
|
|
||
| public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | ||
| const variableNameWalker = new VariableNameWalker(sourceFile, this.getOptions()); | ||
| return this.applyWithWalker(variableNameWalker); | ||
| return this.applyWithFunction(sourceFile, (ctx) => walk(ctx, parseOptions(this.getOptions().ruleArguments))); |
There was a problem hiding this comment.
no need for this.getOptions(), use this.ruleArguments
src/rules/variableNameRule.ts
Outdated
|
|
||
| public static FORMAT_FAILURE = "variable name must be in camelcase or uppercase"; | ||
| public static KEYWORD_FAILURE = "variable name clashes with keyword/type"; | ||
|
|
||
| public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | ||
| const variableNameWalker = new VariableNameWalker(sourceFile, this.getOptions()); | ||
| return this.applyWithWalker(variableNameWalker); | ||
| return this.applyWithFunction(sourceFile, (ctx) => walk(ctx, parseOptions(this.getOptions().ruleArguments))); |
There was a problem hiding this comment.
and as always, why not pass options as third parameter to applyWithFunction and access it via ctx.options?
You would save a closure and a (unnecessary) function call.
PR checklist
Overview of change:
Just a refactor.