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

Jump to bottom

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 •
edited

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 mentioned this pull request Oct 5, 2019
Closed
@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.

mdjermanovic
mdjermanovic reviewed Oct 12, 2019
View reviewed changes
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
@cherryblossom000
Update: Curly multi-or-nest flagging semis on next line (fixes #12370)
e7f4603
@cherryblossom000
Chore: Add more tests to the multi-or-nest option for curly
62e19fe 
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
mdjermanovic
mdjermanovic approved these changes Oct 14, 2019
View reviewed changes
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!

kaicataldo
kaicataldo approved these changes Oct 15, 2019
View reviewed changes
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!

@snyk-bot snyk-bot mentioned this pull request Dec 1, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Dec 11, 2019
Merged
@snyk-bot snyk-bot mentioned this pull request Feb 13, 2020
Merged
@snyk-bot snyk-bot mentioned this pull request Feb 27, 2020
Merged
@a-martynovich a-martynovich mentioned this pull request Mar 10, 2020
Merged
This was referenced Mar 11, 2020
Merged
Open
This was referenced Mar 26, 2020
Merged
Merged
Merged
Open
This was referenced Apr 3, 2020
Merged
Merged
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 1, 2020
@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 May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mdjermanovic mdjermanovic mdjermanovic approved these changes

@kaicataldo kaicataldo Awaiting requested review from kaicataldo

Assignees
No one assigned
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
Milestone
No milestone
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
@cherryblossom000 @mdjermanovic @kaicataldo