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

tools: enable one-var-declaration-per-line ESLint rule #11462

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Feb 19, 2017

This rule enforces new lines around variable declarations. It is
configured to spot only variables that are initialized.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

benchmark, lib, test, tools

This rule enforces new lines around variable declarations. It is
configured to spot only variables that are initialized.
@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module. labels Feb 19, 2017
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

@hiroppy
Copy link
Member

hiroppy commented Feb 19, 2017

@@ -12,14 +12,14 @@ var gargs = [1, 2, 3];

function main(conf) {

var args, n = +conf.n;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: any reason for putting this in for block instead of keeping it here on a separate line?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's clearer to declare it in the block where it's used

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but var is not block scoped so I think this only create confusion for newbies. Can const be used instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I'll change it to const.

targos added a commit to targos/node that referenced this pull request Feb 21, 2017
This rule enforces new lines around variable declarations. It is
configured to spot only variables that are initialized.

PR-URL: nodejs#11462
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos
Copy link
Member Author

targos commented Feb 21, 2017

Landed in 1934686

@targos targos closed this Feb 21, 2017
@targos targos deleted the eslint-one-var-decl branch February 21, 2017 12:02
addaleax pushed a commit that referenced this pull request Feb 22, 2017
This rule enforces new lines around variable declarations. It is
configured to spot only variables that are initialized.

PR-URL: #11462
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@italoacasas italoacasas mentioned this pull request Feb 25, 2017
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

This would require a backport PR to land in v6 or v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. readline Issues and PRs related to the built-in readline module. repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants