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

Refactor variable-name rule #2355

Merged
merged 2 commits into from
Mar 24, 2017
Merged

Refactor variable-name rule #2355

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df436d1
Refactor 'variable-name'
andy-hanson Mar 16, 2017
0e0ae9c
Respond to PR comments
andy-hanson Mar 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 102 additions & 94 deletions src/rules/variableNameRule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,14 @@
* limitations under the License.
*/

// tslint:disable object-literal-sort-keys

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(", ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


const OPTION_LEADING_UNDERSCORE = "allow-leading-underscore";
const OPTION_TRAILING_UNDERSCORE = "allow-trailing-underscore";
Expand All @@ -28,7 +31,6 @@ const OPTION_CHECK_FORMAT = "check-format";
const OPTION_ALLOW_PASCAL_CASE = "allow-pascal-case";

export class Rule extends Lint.Rules.AbstractRule {
/* tslint:disable:object-literal-sort-keys */
public static metadata: Lint.IRuleMetadata = {
ruleName: "variable-name",
description: "Checks variable names for various errors.",
Expand All @@ -39,8 +41,8 @@ export class Rule extends Lint.Rules.AbstractRule {
* \`"${OPTION_LEADING_UNDERSCORE}"\` allows underscores at the beginning (only has an effect if "check-format" specified)
* \`"${OPTION_TRAILING_UNDERSCORE}"\` allows underscores at the end. (only has an effect if "check-format" specified)
* \`"${OPTION_ALLOW_PASCAL_CASE}"\` allows PascalCase in addition to camelCase.
* \`"${OPTION_BAN_KEYWORDS}"\`: disallows the use of certain TypeScript keywords (\`any\`, \`Number\`, \`number\`, \`String\`,
\`string\`, \`Boolean\`, \`boolean\`, \`undefined\`) as variable or parameter names.`,
* \`"${OPTION_BAN_KEYWORDS}"\`: disallows the use of certain TypeScript keywords as variable or parameter names.
* These are: ${bannedKeywordsStr}`,
options: {
type: "array",
items: {
Expand All @@ -60,130 +62,136 @@ export class Rule extends Lint.Rules.AbstractRule {
type: "style",
typescriptOnly: false,
};
/* tslint:enable:object-literal-sort-keys */

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)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for this.getOptions(), use this.ruleArguments

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

class VariableNameWalker extends Lint.RuleWalker {
private shouldBanKeywords: boolean;
private shouldCheckFormat: boolean;

constructor(sourceFile: ts.SourceFile, options: Lint.IOptions) {
super(sourceFile, options);

this.shouldBanKeywords = this.hasOption(OPTION_BAN_KEYWORDS);
interface Options {
banKeywords: boolean;
checkFormat: boolean;
leadingUnderscore: boolean;
trailingUnderscore: boolean;
allowPascalCase: boolean;
}
function parseOptions(ruleArguments: string[]): Options {
const banKeywords = hasOption(OPTION_BAN_KEYWORDS);
return {
banKeywords,
// check variable name formatting by default if no options are specified
this.shouldCheckFormat = !this.shouldBanKeywords || this.hasOption(OPTION_CHECK_FORMAT);
checkFormat: !banKeywords || hasOption(OPTION_CHECK_FORMAT),
leadingUnderscore: hasOption(OPTION_LEADING_UNDERSCORE),
trailingUnderscore: hasOption(OPTION_TRAILING_UNDERSCORE),
allowPascalCase: hasOption(OPTION_ALLOW_PASCAL_CASE),
};

function hasOption(name: string): boolean {
return ruleArguments.indexOf(name) !== -1;
}
}

public visitBindingElement(node: ts.BindingElement) {
if (node.name.kind === ts.SyntaxKind.Identifier) {
const identifier = node.name as ts.Identifier;
this.handleVariableNameKeyword(identifier);
// A destructuring pattern that does not rebind an expression is always an alias, e.g. `var {Foo} = ...;`.
// Only check if the name is rebound (`var {Foo: bar} = ...;`).
if (node.parent !== undefined && node.parent.kind !== ts.SyntaxKind.ObjectBindingPattern || node.propertyName) {
this.handleVariableNameFormat(identifier, node.initializer);
function walk(ctx: Lint.WalkContext<void>, options: Options): void {
return ts.forEachChild(ctx.sourceFile, function cb(node: ts.Node): void {
switch (node.kind) {
case ts.SyntaxKind.BindingElement: {
const { initializer, name, propertyName } = node as ts.BindingElement;
if (name.kind === ts.SyntaxKind.Identifier) {
handleVariableNameKeyword(name);
// A destructuring pattern that does not rebind an expression is always an alias, e.g. `var {Foo} = ...;`.
// Only check if the name is rebound (`var {Foo: bar} = ...;`).
if (node.parent!.kind !== ts.SyntaxKind.ObjectBindingPattern || propertyName) {
handleVariableNameFormat(name, initializer);
}
}
break;
}
}
super.visitBindingElement(node);
}

public visitParameterDeclaration(node: ts.ParameterDeclaration) {
if (node.name.kind === ts.SyntaxKind.Identifier) {
const identifier = node.name as ts.Identifier;
this.handleVariableNameFormat(identifier, undefined /* parameters may not alias */);
this.handleVariableNameKeyword(identifier);
case ts.SyntaxKind.VariableStatement:
// skip 'declare' keywords
if (Lint.hasModifier(node.modifiers, ts.SyntaxKind.DeclareKeyword)) {
return;
}
break;

case ts.SyntaxKind.Parameter:
case ts.SyntaxKind.PropertyDeclaration:
case ts.SyntaxKind.VariableDeclaration: {
const { name, initializer } = node as ts.ParameterDeclaration | ts.PropertyDeclaration | ts.VariableDeclaration;
if (name.kind === ts.SyntaxKind.Identifier) {
handleVariableNameFormat(name, initializer);
// do not check property declarations for keywords, they are allowed to be keywords
if (node.kind !== ts.SyntaxKind.PropertyDeclaration) {
handleVariableNameKeyword(name);
}
}
break;
}
}
super.visitParameterDeclaration(node);
}

public visitPropertyDeclaration(node: ts.PropertyDeclaration) {
if (node.name != null && node.name.kind === ts.SyntaxKind.Identifier) {
const identifier = node.name as ts.Identifier;
this.handleVariableNameFormat(identifier, node.initializer);
// do not check property declarations for keywords, they are allowed to be keywords
}
super.visitPropertyDeclaration(node);
}
return ts.forEachChild(node, cb);
});

public visitVariableDeclaration(node: ts.VariableDeclaration) {
if (node.name.kind === ts.SyntaxKind.Identifier) {
const identifier = node.name as ts.Identifier;
this.handleVariableNameFormat(identifier, node.initializer);
this.handleVariableNameKeyword(identifier);
function handleVariableNameFormat(name: ts.Identifier, initializer?: ts.Expression): void {
if (!options.checkFormat) {
return;
}
super.visitVariableDeclaration(node);
}

public visitVariableStatement(node: ts.VariableStatement) {
// skip 'declare' keywords
if (!Lint.hasModifier(node.modifiers, ts.SyntaxKind.DeclareKeyword)) {
super.visitVariableStatement(node);
const { text } = name;
if (initializer && isAlias(text, initializer)) {
return;
}
}

private isAlias(name: ts.Identifier, initializer: ts.Expression): boolean {
if (initializer.kind === ts.SyntaxKind.PropertyAccessExpression) {
return (initializer as ts.PropertyAccessExpression).name.text === name.text;
} else if (initializer.kind === ts.SyntaxKind.Identifier) {
return (initializer as ts.Identifier).text === name.text;
if (!isCamelCase(text, options) && !isUpperCase(text)) {
ctx.addFailureAtNode(name, Rule.FORMAT_FAILURE);
}
return false;
}

private handleVariableNameFormat(name: ts.Identifier, initializer?: ts.Expression) {
const variableName = name.text;

if (initializer && this.isAlias(name, initializer)) {
return;
}

if (this.shouldCheckFormat && !this.isCamelCase(variableName) && !isUpperCase(variableName)) {
this.addFailureAtNode(name, Rule.FORMAT_FAILURE);
function handleVariableNameKeyword(name: ts.Identifier): void {
if (options.banKeywords && BANNED_KEYWORDS.has(name.text)) {
ctx.addFailureAtNode(name, Rule.KEYWORD_FAILURE);
}
}
}

private handleVariableNameKeyword(name: ts.Identifier) {
const variableName = name.text;

if (this.shouldBanKeywords && BANNED_KEYWORDS.indexOf(variableName) !== -1) {
this.addFailureAtNode(name, Rule.KEYWORD_FAILURE);
}
function isAlias(name: string, initializer: ts.Expression): boolean {
switch (initializer.kind) {
case ts.SyntaxKind.PropertyAccessExpression:
return (initializer as ts.PropertyAccessExpression).name.text === name;
case ts.SyntaxKind.Identifier:
return (initializer as ts.Identifier).text === name;
default:
return false;
}
}

private isCamelCase(name: string) {
const firstCharacter = name.charAt(0);
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const lastCharacter = name.charAt(name.length - 1);
const middle = name.substr(1, name.length - 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be simplified to name.slice(1, -1)


if (name.length <= 0) {
return true;
}
if (!this.hasOption(OPTION_LEADING_UNDERSCORE) && firstCharacter === "_") {
return false;
}
if (!this.hasOption(OPTION_TRAILING_UNDERSCORE) && lastCharacter === "_") {
return false;
}
if (!this.hasOption(OPTION_ALLOW_PASCAL_CASE) && !isLowerCase(firstCharacter)) {
return false;
}
return middle.indexOf("_") === -1;
if (name.length <= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return true;
}
if (!options.leadingUnderscore && firstCharacter === "_") {
return false;
}
if (!options.trailingUnderscore && lastCharacter === "_") {
return false;
}
if (!options.allowPascalCase && !isLowerCase(firstCharacter)) {
return false;
}
return middle.indexOf("_") === -1;
}

function isLowerCase(name: string) {
function isLowerCase(name: string): boolean {
return name === name.toLowerCase();
}

function isUpperCase(name: string) {
function isUpperCase(name: string): boolean {
return name === name.toUpperCase();
}