Skip to content

Update: single- and multiline const, let, var statements (fixes #10721) - #10919

Merged
not-an-aardvark merged 1 commit into
eslint:masterfrom
neemzy:master
Dec 8, 2018
Merged

Update: single- and multiline const, let, var statements (fixes #10721)#10919
not-an-aardvark merged 1 commit into
eslint:masterfrom
neemzy:master

Conversation

@neemzy

@neemzy neemzy commented Oct 5, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

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

#10721

What changes did you make? (Give an overview)

  • Added new STATEMENT_TYPEs for multiline variable declarations (multiline-const, multiline-let, multiline-var, singleline-const, singleline-let, single-line-var)

@jsf-clabot

jsf-clabot commented Oct 5, 2018 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated Bot added the triage An ESLint team member will look at this issue soon label Oct 5, 2018
@neemzy neemzy changed the title Upgrade: added separate statement types for multiline variable declarations in padding-line-between-statements rule (fixes #10721) Upgrade: discriminated multiline variable declarations (fixes #10721) Oct 5, 2018
@aladdin-add aladdin-add added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Oct 5, 2018
@not-an-aardvark not-an-aardvark mentioned this pull request Oct 30, 2018
Closed
@neemzy neemzy changed the title Upgrade: discriminated multiline variable declarations (fixes #10721) Upgrade: discriminated singe-line and multiline variable declarations (fixes #10721) Nov 5, 2018
@neemzy

neemzy commented Nov 5, 2018

Copy link
Copy Markdown
Contributor Author

PR was updated as requested in #10721: singleline-* STAMEMENT_TYPEs were also added to allow for discrimination while preserving backwards compatibility.

@neemzy neemzy changed the title Upgrade: discriminated singe-line and multiline variable declarations (fixes #10721) Upgrade: single- and multiline const, let, var statements (fixes #10721) Nov 5, 2018
@neemzy

neemzy commented Nov 12, 2018

Copy link
Copy Markdown
Contributor Author

@platinumazure @not-an-aardvark Is that OK for you guys? :)

platinumazure
platinumazure suggested changes Nov 22, 2018
View reviewed changes

@platinumazure platinumazure left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This implementation is looking good so far. Sorry for losing track of this.

Just one request: Could you add some tests that use the multiline-var option with a single line var statement (and vice versa, and for let/const) which show that the "wrong" options don't lint for those cases? Thanks!

@neemzy

neemzy commented Nov 22, 2018

Copy link
Copy Markdown
Contributor Author

@platinumazure Done, thanks!

platinumazure
platinumazure approved these changes Nov 22, 2018
View reviewed changes

@platinumazure platinumazure left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks! Just want some more eyes on this.

@platinumazure

platinumazure commented Nov 22, 2018

Copy link
Copy Markdown
Member

Oh, can you take a look at the Contributor License Agreement issue? We can't merge this without you signing the CLA. Thanks!

@neemzy

neemzy commented Nov 22, 2018

Copy link
Copy Markdown
Contributor Author

@platinumazure Hmm, that's strange, I signed it when I first opened the PR and can't sign it again if I check the details (fields are disabled and there is no submit button anyway). See https://imgur.com/a/ROMQPZy

@not-an-aardvark

not-an-aardvark commented Nov 23, 2018

Copy link
Copy Markdown
Member

@neemzy It seems like your git commit was created with a different email address (tpanier at synbioz dot com) than the one associated with your GitHub account, which is why the CLA check is now failing. One way to fix it would be to amend the commit to make sure the correct email is used, or to add the other email address to your GitHub account.

@neemzy

neemzy commented Nov 23, 2018

Copy link
Copy Markdown
Contributor Author

@not-an-aardvark Ah, right, my bad. It's fixed!

@platinumazure platinumazure 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 Nov 23, 2018
@neemzy

neemzy commented Dec 3, 2018

Copy link
Copy Markdown
Contributor Author

@platinumazure Are we waiting for more approvals from the team? :)

@nzakas nzakas changed the title Upgrade: single- and multiline const, let, var statements (fixes #10721) Update: single- and multiline const, let, var statements (fixes #10721) Dec 7, 2018
nzakas
nzakas approved these changes Dec 7, 2018
View reviewed changes

@nzakas nzakas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@nzakas

nzakas commented Dec 7, 2018

Copy link
Copy Markdown
Member

@kaicataldo @not-an-aardvark any objections to merging this?

not-an-aardvark
not-an-aardvark approved these changes Dec 8, 2018
View reviewed changes

@not-an-aardvark not-an-aardvark left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks!

@not-an-aardvark
not-an-aardvark merged commit 4b0f517 into eslint:master Dec 8, 2018
@neemzy

neemzy commented Dec 11, 2018

Copy link
Copy Markdown
Contributor Author

Thanks! :)

@eslint-deprecated eslint-deprecated Bot locked and limited conversation to collaborators Jun 7, 2019
@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 Jun 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@nzakas nzakas nzakas approved these changes
+2 more reviewers
@platinumazure platinumazure platinumazure approved these changes
@not-an-aardvark not-an-aardvark not-an-aardvark approved these changes
Reviewers whose approvals may not affect merge requirements

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 enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@neemzy @jsf-clabot @platinumazure @not-an-aardvark @nzakas @aladdin-add