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
Merged

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

merged 2 commits into from
Nov 1, 2019

Conversation

cherryblossom000
Copy link
Contributor

@cherryblossom000 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-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon 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
Copy link
Member

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()
;
}

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Oct 8, 2019
@cherryblossom000
Copy link
Contributor Author

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

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.

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
Copy link
Member

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
Copy link
Member

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 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!

Copy link
Member

@kaicataldo kaicataldo 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 for contributing!

@mdjermanovic mdjermanovic removed the enhancement This change enhances an existing feature of ESLint label Oct 22, 2019
@cherryblossom000
Copy link
Contributor Author

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

@kaicataldo kaicataldo merged commit 990065e into eslint:master Nov 1, 2019
@kaicataldo
Copy link
Member

Thanks for contributing to ESLint!

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
None yet
Development

Successfully merging this pull request may close these issues.

curly: multi-or-nest option reports an error for a single-statment block with a semicolon on the next line
3 participants