Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

no-fallthrough rule's bypass comment behavior isn't intuitive when case followed by curly braces #9080

Closed
7sDream opened this issue Aug 6, 2017 · 1 comment
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules

Comments

@7sDream
Copy link

7sDream commented Aug 6, 2017

Tell us about your environment

  • ESLint Version: v4.4.0
  • Node Version: 8.2.1
  • npm Version: 5.3.0

What parser (default, Babel-ESLint, etc.) are you using?

Default parser

Please show your full configuration:

Configuration
{
    "env": {
        "es6": true,
        "node": true
    },
    "extends": "eslint:recommended",
    "parserOptions": {
        "sourceType": "module"
    },
    "rules": {
        "no-fallthrough": "error"
    }
}

What did you do? Please include the actual source code causing the issue.

let someVar = 0

// This one cause a eslint error because of no break in first `case`
switch (someVar) {
case 0: {
    someVar = 1
}
default: {
    someVar = 2
}
}

// this one is fine, bypass comment added JUST before second `case`
switch (someVar) {
case 0: 
    someVar = 1
    // fall through
default:
    someVar = 2
}

// this one is fine, bypass comment added JUST before second `case`
switch (someVar) {
case 0: {
    someVar = 1
}
// fall through
default: {
    someVar = 2
}
}

// This one cause a eslint error, although bypass comment is added
switch (someVar) {
case 0: {
    someVar = 1
    // fall through
}
default: {
    someVar = 2
}
}

What did you expect to happen?

The last switch example will not cause a lint error.

What actually happened? Please include the actual, raw output from ESLint.

The last switch example does cause a lint error.

> eslint-test@1.0.0 test /home/i7sdream/code/eslint-test
> ./node_modules/eslint/bin/eslint.js .


/home/i7sdream/code/eslint-test/index.js
   8:1  error  Expected a 'break' statement before 'default'  no-fallthrough
  39:1  error  Expected a 'break' statement before 'default'  no-fallthrough

✖ 2 problems (2 errors, 0 warnings)

npm ERR! Test failed.  See above for more details.

some of my views

Maybe the rules now only check if there is a bypass comment in the line JUST before each case, keyword, but I like to enclose around the case code with curly braces, so the rule doesn't recognize the bypass comment.

But, IMHO, when enclose with curly braces, bypass comment written in the last line in the, let's say, BLOCK of the first case is more intuitive. (last example)

So maybe we can change the rule to determine whether the last sentence in the BLOCK of the first case is bypass comment?

This change is backward compatible, because when there is no curly braces, the last sentence of the first case's BLOCK is also the prev line of the second case, because the BLOCK is all sentences of first case, in this situation.

Any comments is welcome, except for "why you like to add braces after case".

:)

@eslintbot eslintbot added the triage An ESLint team member will look at this issue soon label Aug 6, 2017
@kaicataldo kaicataldo added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Aug 7, 2017
@not-an-aardvark
Copy link
Member

Thanks for your interest in improving ESLint. Unfortunately, it looks like this issue didn't get consensus from the team, so I'm closing it. We define consensus as having three 👍s from team members, as well as a team member willing to champion the proposal. This is a high bar by design -- we can't realistically accept and maintain every feature request in the long term, so we only accept feature requests which are useful enough that there is consensus among the team that they're worth adding.

Since ESLint is pluggable and can load custom rules at runtime, the lack of consensus among the ESLint team doesn't need to be a blocker for you using this in your project, if you'd find it useful. It just means that you would need to implement the rule yourself, rather than using a bundled rule that is packaged with ESLint.

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jul 19, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jul 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

4 participants