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: single- and multiline const, let, var statements (fixes #10721) #10919

Merged
merged 1 commit into from Dec 8, 2018
Merged

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

merged 1 commit into from Dec 8, 2018

Conversation

neemzy
Copy link
Contributor

@neemzy neemzy commented Oct 5, 2018 •
edited

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

jsf-clabot commented Oct 5, 2018 •
edited

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
Copy link
Contributor Author

neemzy commented Nov 5, 2018

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
Copy link
Contributor Author

neemzy commented Nov 12, 2018

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

platinumazure
platinumazure requested changes Nov 22, 2018
View reviewed changes
Copy link
Member

@platinumazure platinumazure left a comment

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
Copy link
Contributor Author

neemzy commented Nov 22, 2018

@platinumazure Done, thanks!

platinumazure
platinumazure approved these changes Nov 22, 2018
View reviewed changes
Copy link
Member

@platinumazure platinumazure 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! Just want some more eyes on this.

@platinumazure
Copy link
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
Copy link
Contributor Author

neemzy commented Nov 22, 2018

@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
Copy link
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
Copy link
Contributor Author

neemzy commented Nov 23, 2018

@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
Copy link
Contributor Author

neemzy commented Dec 3, 2018

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

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM

@nzakas
Copy link
Member

nzakas commented Dec 7, 2018

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

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

@not-an-aardvark not-an-aardvark 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!

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

neemzy commented Dec 11, 2018

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

@platinumazure platinumazure platinumazure approved these changes

@not-an-aardvark not-an-aardvark not-an-aardvark approved these changes

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.

None yet
6 participants
@neemzy @jsf-clabot @platinumazure @not-an-aardvark @nzakas @aladdin-add