-
Notifications
You must be signed in to change notification settings - Fork 889
Added a no-duplicate-switch-case rule #2937
Changes from 1 commit
51eb1f6
427b448
a93d074
b26394d
43573b2
7d33cc0
8ca99cd
1bfa307
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,72 @@ | ||
/** | ||
* @license | ||
* Copyright 2017 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 "../index"; | ||
|
||
export class Rule extends Lint.Rules.AbstractRule { | ||
public static metadata: Lint.IRuleMetadata = { | ||
description: "Prevents duplicate cases in switch statements.", | ||
optionExamples: [true], | ||
options: null, | ||
optionsDescription: "", | ||
ruleName: "no-duplicate-switch-case", | ||
type: "functionality", | ||
typescriptOnly: false, | ||
}; | ||
|
||
public static readonly FAILURE_STRING_FACTORY = (text: string) => `Duplicate switch case: '${text}'.`; | ||
|
||
public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { | ||
return this.applyWithFunction(sourceFile, walk); | ||
} | ||
} | ||
|
||
const walk = (ctx: Lint.WalkContext<void>): void => { | ||
const cb = (node: ts.Node): void => { | ||
if (node.kind === ts.SyntaxKind.CaseBlock) { | ||
visitCaseBlock(node as ts.CaseBlock); | ||
} | ||
|
||
ts.forEachChild(node, cb); | ||
}; | ||
|
||
const visitCaseBlock = (() => { | ||
const previousCases = new Set<string>(); | ||
|
||
return (node: ts.CaseBlock): void => { | ||
previousCases.clear(); | ||
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. no need to return a closure or clear the set. 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. It's a minor performance optimization to keep the same set across iterations. Do you consider the lessened clarity not worth it? 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. I would guess clearing the set is as expensive as creating a new one. |
||
|
||
for (const clause of node.clauses) { | ||
if (clause.kind === ts.SyntaxKind.DefaultClause) { | ||
continue; | ||
} | ||
|
||
const text = clause.expression.getText(); | ||
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. consider passing |
||
if (!previousCases.has(text)) { | ||
previousCases.add(text); | ||
continue; | ||
} | ||
|
||
ctx.addFailureAtNode(clause.expression, Rule.FAILURE_STRING_FACTORY(text)); | ||
} | ||
}; | ||
})(); | ||
|
||
ts.forEachChild(ctx.sourceFile, cb); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
const withNumber = (value: number) => { | ||
switch (value): | ||
case 0: | ||
break; | ||
|
||
case 1: | ||
break; | ||
|
||
case 0: | ||
~ [error % ("0")] | ||
case 1: { | ||
~ [error % ("1")] | ||
break; | ||
} | ||
}; | ||
|
||
class WithString { | ||
constructor(param: string) { | ||
switch (param) { | ||
case "aaa": | ||
break; | ||
|
||
case "bbb": | ||
case "ccc": | ||
break; | ||
|
||
case "bbb": | ||
~~~~~ [error % ('"bbb"')] | ||
case "ddd": | ||
switch (param.length) { | ||
case 0: | ||
break; | ||
|
||
case 0: | ||
~ [error % ("0")] | ||
case 1: | ||
break; | ||
|
||
default: | ||
break; | ||
} | ||
|
||
case "eee": | ||
case "eee": | ||
~~~~~ [error % ('"bbb"')] | ||
break; | ||
|
||
case "default": | ||
break; | ||
|
||
case 0: | ||
break; | ||
|
||
case "1": | ||
break; | ||
|
||
default: | ||
break; | ||
} | ||
} | ||
|
||
test(obj: object) { | ||
switch (obj) { | ||
case undefined: | ||
break; | ||
|
||
case null: | ||
case Infinity: | ||
break; | ||
|
||
case this: | ||
case null: | ||
~~~~ [error % ("null")] | ||
break; | ||
|
||
default: | ||
break; | ||
} | ||
} | ||
} | ||
|
||
[error]: Duplicate switch case: '%s'. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"rules": { | ||
"no-duplicate-switch-case": [true] | ||
} | ||
} |
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.
consider converting these lambdas to regular function declarations
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.
Is there a reason behind the preference for regular function declarations? Elsewhere I've seen folks prefer arrow lambdas for the saner scoping & lack of hoisting weirdness.
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.
The rest of the codebase seems to follow the pattern to use function declarations where possible and only use arrow functions where this reference matters.
I prefer regular functions because I think it's easier to notice while scanning through the code