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

Add 'switch-final-break' rule #2804

Merged
merged 5 commits into from
May 26, 2017

Conversation

andy-hanson
Copy link
Contributor

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

Added the 'switch-final-break' lint rule, which:

  • For a case, enforces 'no-switch-case-fall-through' even for the last case.
  • For a default: clause, forbids a useless break;.

CHANGELOG.md entry:

[new-rule] switch-final-break

@andy-hanson andy-hanson force-pushed the switch-final-break branch 2 times, most recently from 95f291b to 80b2714 Compare May 21, 2017 23:09
Enforces consistent use of 'break;' in the final clause of a 'switch' statement.
A 'default' clause should never have a 'break'.
A 'case' clause should always have a 'break' (or other control flow exit) even if it is the last clause.`,
optionsDescription: "Not configurable.",
Copy link
Contributor

Choose a reason for hiding this comment

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

consider making this rule configurable with options like "always" and "never" (and if you really want it, you can add another option to handle default different. although I don't understand why the default clause should be handled differently at all).

For the record, I'd prefer setting this rule to "never" in this repository to avoid unnecessary statements.

public static metadata: Lint.IRuleMetadata = {
ruleName: "switch-final-break",
description: Lint.Utils.dedent`
Forbids the final clause of a switch statement to have an unnecessary \`break;\`.`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be updated to sth like: Forbids or requires ...

}
}

function last<T>(arr: ReadonlyArray<T>): T | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why you need this one, because you still have to check for undefined (instead of arr.length without this function), but I don't have a problem with it being there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better to ask forgiveness than permission. I'd rather check for an undefined return value than to have some test for whether the function I'm about to call should return a defined value.

@@ -0,0 +1,46 @@
switch (x) {
Copy link
Contributor

Choose a reason for hiding this comment

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

add a test where the last clause is empty


function check(node: ts.CaseBlock): void {
const clause = last(node.clauses);
if (clause === undefined || clause.statements.length === 0) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

if always is specified, the rule should also complain if the final clause is empty

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants