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

Update: curly multi-or-nest flagging semis on next line (fixes #12370) #12378

Merged
merged 2 commits into from Nov 1, 2019

Conversation

@cherryblossom000
Copy link
Contributor

cherryblossom000 commented Oct 5, 2019

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

[x] Bug fix

Fixes #12370.

What changes did you make? (Give an overview)

I made the isOneLiner function use the last token excluding the semicolon instead of just the last token, like the isCollapsedOneLiner function, which stopped multi-or-nest flagging code like the following as one line.

console.log('foo')
;

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

No.

@eslint eslint bot added the triage label Oct 5, 2019
@cherryblossom000 cherryblossom000 changed the title Fix: Curly multi-or-nest option flagging semis on next line (fixes 12370) Fix: Curly multi-or-nest option flagging semis on next line (fixes #12370) Oct 5, 2019
@cherryblossom000 cherryblossom000 changed the title Fix: Curly multi-or-nest option flagging semis on next line (fixes #12370) Fix: Curly multi-or-nest flagging semis on next line (fixes #12370) Oct 5, 2019
@mdjermanovic

This comment has been minimized.

Copy link
Member

mdjermanovic commented Oct 8, 2019

Thanks for the PR! Marking as accepted, as the issue is accepted.

Can this change also produce more warnings? For example, in some rare situations like this:

/* eslint curly: ["error", "multi-or-nest"] */

if (foo) {
  bar()
;
}
@cherryblossom000

This comment has been minimized.

Copy link
Contributor Author

cherryblossom000 commented Oct 9, 2019

Yes, this change does produce those warnings. I added more tests for that.

Copy link
Member

mdjermanovic left a comment

Thanks for the detailed tests!

Could you please also change the commit message and PR title to start with "Update:" instead of "Fix:"? It's because the change can generate more warning and thus requires a semver-minor release.

lib/rules/curly.js Show resolved Hide resolved
@cherryblossom000 cherryblossom000 changed the title Fix: Curly multi-or-nest flagging semis on next line (fixes #12370) Update: Curly multi-or-nest flagging semis on next line (fixes #12370) Oct 12, 2019
Check that multi-or-nest it fixes cases like
if (foo) {
  doSomething()
  ;
}

and that it ignores cases with an empty statement like
if (foo)
;
@mdjermanovic

This comment has been minimized.

Copy link
Member

mdjermanovic commented Oct 13, 2019

One small detail I missed before, sorry - 'curly' is the rule's name and should start with a lowercase in commit message and PR title.

Other than that, everything else LGTM!

@mdjermanovic

This comment has been minimized.

Copy link
Member

mdjermanovic commented Oct 13, 2019

One small detail I missed before, sorry - 'curly' is the rule's name and should start with a lowercase in commit message and PR title.

Other than that, everything else LGTM!

Actually, please disregard the 'commit message' part.

It's okay to change just the PR title because this PR has more than 1 commit, so the PR title will be used as the merge commit message.

@cherryblossom000 cherryblossom000 changed the title Update: Curly multi-or-nest flagging semis on next line (fixes #12370) Update: curly multi-or-nest flagging semis on next line (fixes #12370) Oct 14, 2019
Copy link
Member

mdjermanovic left a comment

LGTM, thanks!

Copy link
Member

kaicataldo left a comment

LGTM. Thanks for contributing!

@cherryblossom000 cherryblossom000 requested a review from kaicataldo Oct 30, 2019
@cherryblossom000

This comment has been minimized.

Copy link
Contributor Author

cherryblossom000 commented Oct 30, 2019

@kaicataldo Sorry, ignore the review request. That was accidental.

@kaicataldo kaicataldo merged commit 990065e into eslint:master Nov 1, 2019
16 checks passed
16 checks passed
Verify Files
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191012.6 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@kaicataldo

This comment has been minimized.

Copy link
Member

kaicataldo commented Nov 1, 2019

Thanks for contributing to ESLint!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.