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

feat: no-case-declarations add suggestions #18388

Merged

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Apr 24, 2024

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[x] Add autofix suggestion to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Fixes #18349

What changes did you make? (Give an overview)

Adds a suggestion fixer to no-case-declarations to add { } brackets around blocks.

Also adds a couple new tests to make sure switches with multiple violating cases report on the right case (consequent).

Is there anything you'd like reviewers to focus on?

I'd considered adding logic for friendlier { } bracket placement but then decided against the added complexity.

@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Apr 24, 2024
@github-actions github-actions bot added the rule Relates to ESLint's core rules label Apr 24, 2024
Copy link

netlify bot commented Apr 24, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 9dced47
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/662967f07277fa0009093e1d

@mdjermanovic mdjermanovic added the accepted There is consensus among the team that this change meets the criteria for inclusion label Apr 24, 2024
mdjermanovic
mdjermanovic previously approved these changes Apr 24, 2024
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Leaving it open as, per our policies, it requires at least one additional approval from any other team member (because this is a new feature), and also a waiting period of 2 days before merging.

@fasttime
Copy link
Member

When a declared identifier is used in another case block, applying the suggestion can change the logic, for example:

// ecmaVersion: "latest"
switch ("foo") {
    case "bar":
        function baz() { }
        break;
    default:
        baz();
}

compared to:

// ecmaVersion: "latest"
switch ("foo") {
    case "bar":
        { function baz() { }
        break; }
    default:
        baz(); // TypeError!
}

I assume this is acceptable for a suggestion, but maybe we should add a unit test to show that we are aware of the risk and we know what we are doing?

Copy link
Member

@fasttime fasttime left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Leaving open to respect the the 2 days period.

@mdjermanovic mdjermanovic merged commit 8485d76 into eslint:main Apr 26, 2024
18 checks passed
@JoshuaKGoldberg JoshuaKGoldberg deleted the no-case-declarations-suggestions branch April 26, 2024 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

Rule Change: Add suggestion fixer to no-case-declarations
3 participants