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 should allow the comment inside of a block when the case is a single block #14701

Closed
bakkot opened this issue Jun 12, 2021 · 5 comments · Fixed by #14702
Closed
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects

Comments

@bakkot
Copy link
Contributor

bakkot commented Jun 12, 2021

What rule do you want to change?

no-fallthrough

Does this change cause the rule to produce more or fewer warnings?

fewer

How will the change be implemented? (New option, new default behavior, etc.)?

new default behavior

Please provide some example code that this change will affect:

switch (foo) {
  case 1: {
    let x = value;
    console.log(value);
    // falls through
  }
  case 2: {
    doSomething();
  }
}

What does the rule currently do for this code?

Error

What will the rule do after it's changed?

Not error

Are you willing to submit a pull request to implement this change?

Yes


This has been brought up before, in #9559 and #9080, and with a PR in #11016. On each of those occasions it's been closed automatically without any interest. But those just asked for the new behavior without really explaining why.

I want to make a case the proposed change is the right behavior. The fallthrough comment is intended to replace a break, which means that a reader of the code will be looking for it where they would expect to find the break. And the convention when using a braced case is that the break occurs within the braces, not outside of it. So that is where a reader should expect to find the comment, not (as eslint currently requires) after the braces. The placement required by the current rule actually makes the comment easier to miss.

@bakkot bakkot added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Jun 12, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jun 12, 2021
@aladdin-add
Copy link
Member

aladdin-add commented Jun 13, 2021

as a workaround, you can write the comment out the block:

switch (foo) {
  case 1: {
    let x = value;
    console.log(value);
  }
  // falls through
  case 2: {
    doSomething();
  }
}

IMHO, it seems a bug, no new option needed. but let's see what others thoughts. :)

@aladdin-add aladdin-add added bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Jun 13, 2021
@aladdin-add aladdin-add moved this from Needs Triage to Feedback Needed in Triage Jun 13, 2021
@mdjermanovic
Copy link
Member

IMHO, it seems a bug, no new option needed. but let's see what others thoughts. :)

Agreed. If consequent consists of only one statement, and that statement is BlockStatement, then the rule should also look for the comment before its closing }. The rule will still allow comment after }.

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 13, 2021
@mdjermanovic mdjermanovic moved this from Feedback Needed to Ready to Implement in Triage Jun 13, 2021
@bakkot
Copy link
Contributor Author

bakkot commented Jun 14, 2021

I'll prepare a PR for this unless someone is already working on it.

bakkot added a commit to bakkot/eslint that referenced this issue Jun 14, 2021
@aladdin-add
Copy link
Member

@bakkot please feel free to open a PR, thanks!

@bakkot
Copy link
Contributor Author

bakkot commented Jun 14, 2021

Done: #14702.

@aladdin-add aladdin-add moved this from Ready to Implement to Pull Request Opened in Triage Jun 14, 2021
@mdjermanovic mdjermanovic linked a pull request Jun 14, 2021 that will close this issue
1 task
Triage automation moved this from Pull Request Opened to Complete Jun 18, 2021
btmills pushed a commit that referenced this issue Jun 18, 2021
* Fix: allow fallthrough comment inside block (fixes #14701)

* address comments
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 16, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

3 participants