-
Notifications
You must be signed in to change notification settings - Fork 889
Adding new rule to disallow non consecutive function overloads #1426
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
/** | ||
* @license | ||
* Copyright 2013 Palantir Technologies, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import * as ts from "typescript"; | ||
|
||
import * as Lint from "../lint"; | ||
|
||
export class Rule extends Lint.Rules.AbstractRule { | ||
|
||
/* tslint:disable:object-literal-sort-keys */ | ||
public static metadata: Lint.IRuleMetadata = { | ||
ruleName: "consecutive-function-overloads", | ||
description: "Enforces function overloads to be consecutive.", | ||
optionsDescription: "Not configurable.", | ||
options: null, | ||
optionExamples: ["true"], | ||
type: "style", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's change this to |
||
}; | ||
/* tslint:enable:object-literal-sort-keys */ | ||
|
||
public static FAILURE_STRING_FACTORY = (name: string) => `All '${name}' signatures should be adjacent`; | ||
|
||
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | ||
return this.applyWithWalker(new NoNonConsecutiveFunctionOverload(sourceFile, this.getOptions())); | ||
} | ||
} | ||
|
||
function isStringOrNumericLiteral(kind: ts.SyntaxKind) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prefer private helper functions to occur at the end of the file, after the walker class please There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, can you refactor this into a type guard? function isLiteralExpression(node: ts.Node): node is ts.LiteralExpression {
return node.kind === ts.SyntaxKind.StringLiteral || node.kind === ts.SyntaxKind.NumericLiteral;
} generally, type guards are preferred over assertions whenever possible. |
||
return kind === ts.SyntaxKind.StringLiteral || kind === ts.SyntaxKind.NumericLiteral; | ||
} | ||
|
||
function getTextOfPropertyName(name: ts.PropertyName): string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you create the type guard I suggest above, this can be a little simpler: switch (name.kind) {
case ts.SyntaxKind.Identifier:
return (name as ts.Identifier).text;
case ts.SyntaxKind.ComputedPropertyName:
const { expression } = (name as ts.ComputedPropertyName);
if (isLiteralExpression(expression)) {
return expression.text;
}
default:
if (isLiteralExpression(name)) {
return name.text;
}
} |
||
switch (name.kind) { | ||
case ts.SyntaxKind.Identifier: | ||
return (<ts.Identifier> name).text; | ||
case ts.SyntaxKind.StringLiteral: | ||
case ts.SyntaxKind.NumericLiteral: | ||
return (<ts.LiteralExpression> name).text; | ||
case ts.SyntaxKind.ComputedPropertyName: | ||
if (isStringOrNumericLiteral((<ts.ComputedPropertyName> name).expression.kind)) { | ||
return (<ts.LiteralExpression> (<ts.ComputedPropertyName> name).expression).text; | ||
} | ||
default: | ||
return undefined; | ||
} | ||
} | ||
|
||
class NoNonConsecutiveFunctionOverload extends Lint.BlockScopeAwareRuleWalker<{}, ScopeInfo> { | ||
|
||
public createScope(): any { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please narrow this return type to |
||
return undefined; | ||
} | ||
|
||
public createBlockScope(): ScopeInfo { | ||
return new ScopeInfo(); | ||
} | ||
|
||
public visitInterfaceDeclaration(node: ts.InterfaceDeclaration): void { | ||
const members = node.members; | ||
for (const member of members) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if (member.name !== undefined) { | ||
const methodName = getTextOfPropertyName(member.name); | ||
if (methodName !== undefined) { | ||
this.handleMethodName(member, methodName); | ||
} | ||
} | ||
} | ||
super.visitInterfaceDeclaration(node); | ||
} | ||
|
||
private handleMethodName(node: ts.Node, methodName: string) { | ||
const currentBlockScope = this.getCurrentBlockScope(); | ||
const lastPosition = currentBlockScope.methodNames.lastIndexOf(methodName); | ||
if (lastPosition >= 0 && lastPosition !== (currentBlockScope.methodNames.length - 1)) { | ||
this.addFailure(this.createFailure(node.getStart(), node.getWidth(), Rule.FAILURE_STRING_FACTORY(methodName))); | ||
} | ||
currentBlockScope.methodNames.push(methodName); | ||
} | ||
} | ||
|
||
class ScopeInfo { | ||
public methodNames: string[] = []; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
// good | ||
|
||
interface i1 { | ||
a(); | ||
a(x: number); | ||
b(); | ||
b(x: string); | ||
} | ||
|
||
interface i2 { | ||
a(); | ||
a(x: number); | ||
a(): void; | ||
b(); | ||
b(x: string); | ||
} | ||
|
||
interface i3 { | ||
a(); | ||
"a"(); | ||
} | ||
|
||
interface i4 { | ||
a(); | ||
["a"](); | ||
} | ||
|
||
|
||
// bad | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add some tests for nested interfaces? interface foo {
a: string;
bar: {
a: number;
...
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! I don't think this code will work for nested interfaces right now since it's only sweeps interfaceDeclarations. I probably would have to check if an interface member is an object and then recurse for them. Do you have any other suggestion? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, I think you'll want to recurse and create a new block scope for the nested member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, take a look at the object-literal-sort-keys rule because it has to handle a similar scenario for nested objects |
||
|
||
interface b1 { | ||
a(); | ||
a(x: number); | ||
b(); | ||
b(x: string); | ||
a(x: string); | ||
~~~~~~~~~~~~~ [All 'a' signatures should be adjacent] | ||
} | ||
|
||
interface b22 { | ||
a(); | ||
a(x: number); | ||
b(); | ||
b(x: string); | ||
a(): void; | ||
~~~~~~~~~~ [All 'a' signatures should be adjacent] | ||
} | ||
|
||
interface b3 { | ||
a(); | ||
12(); | ||
"a"(); | ||
~~~~~~ [All 'a' signatures should be adjacent] | ||
12(); | ||
~~~~~ [All '12' signatures should be adjacent] | ||
} | ||
|
||
interface b4 { | ||
a(); | ||
b(): void; | ||
["a"](v: number): void; | ||
~~~~~~~~~~~~~~~~~~~~~~~ [All 'a' signatures should be adjacent] | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: remove trailing whitespace and add an EOF newline |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"rules": { | ||
"consecutive-function-overloads": true | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: please add an EOF newline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about naming this
adjacent-overload-signatures
?make sure to name the walker class appropriately as well; in this case it would be
AdjacentOverloadSignaturesWalker