tools: alignment of variable assignments that span multiple lines#6242

Closed

Trott wants to merge 2 commits into

nodejs:masterfrom

Trott commented Apr 16, 2016

Member

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

tools, test, lib

Description of change

Since I always mess up the alignment in variable assignments, forcing
@bnoordhuis to provide nits for my pull requests, let's have a lint rule
so that I can be told by the linter instead.

Trott added 2 commits

April 15, 2016 21:21


          lib,test,tools: alignment on variable assignments

9c63cc6

Correct alignment on variable assignments that span multiple lines in
preparation for lint rule to enforce such alignment.


          tools: lint for alignment of variable assignments

24b1589

Enforce alignment/indentation on variable assignments that span multiple
lines.

Trott added the tools label

Trott commented Apr 16, 2016

Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/2287/

Trott commented Apr 16, 2016

Member Author

CI is green except for that pesky test-http-agent test.

bnoordhuis reviewed

View reviewed changes

test/parallel/test-tls-cert-regression.js

+              KHg=
+              -----END CERTIFICATE-----`;
+              var key =

bnoordhuis Apr 16, 2016 •

edited

Loading

Member

You could make these const while you're here. Just a suggestion, though.

EDIT: OTOH, that's mixing disparate changes. Feel free to disregard.

bnoordhuis commented Apr 16, 2016

Member

Nice. LGTM.

mscdex reviewed

View reviewed changes

tools/eslint-rules/align-multiline-assignment.js

+                  context.report(node, msg);
+              }
+              function testDecleration(context, node) {

mscdex Apr 16, 2016

Contributor

s/testDecleration/testDeclaration/

jbergstroem mentioned this pull request

tools: consistency of colon spacing in .eslintrc #6210

Closed

2 tasks

jbergstroem commented Apr 16, 2016

Member

LGTM

1 similar comment

jasnell commented Apr 18, 2016

Member

LGTM

jasnell commented Apr 19, 2016

Member

Fixing the type @mscdex points out on landing...

jasnell pushed a commit that referenced this pull request


          lib,test,tools: alignment on variable assignments

Correct alignment on variable assignments that span multiple lines in
preparation for lint rule to enforce such alignment.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell pushed a commit that referenced this pull request


          tools: lint for alignment of variable assignments

ca69833

Enforce alignment/indentation on variable assignments that span multiple
lines.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Apr 19, 2016

Member

Landed in 3160073 and ca69833

jasnell closed this

jasnell added the lts-watch-v4.x label

MylesBorins pushed a commit that referenced this pull request


          tools: lint for alignment of variable assignments

464867c

Enforce alignment/indentation on variable assignments that span multiple
lines.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request


          tools: lint for alignment of variable assignments

f0ff2f0

Enforce alignment/indentation on variable assignments that span multiple
lines.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

Trott added a commit to Trott/io.js that referenced this pull request


          lib,test,tools: alignment on variable assignments

ec729c2

Correct alignment on variable assignments that span multiple lines in
preparation for lint rule to enforce such alignment.

PR-URL: nodejs#6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

Trott added a commit to Trott/io.js that referenced this pull request


          tools: lint for alignment of variable assignments

b625e22

Enforce alignment/indentation on variable assignments that span multiple
lines.

PR-URL: nodejs#6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request


          lib,test,tools: alignment on variable assignments

799f414

Correct alignment on variable assignments that span multiple lines in
preparation for lint rule to enforce such alignment.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request


          tools: lint for alignment of variable assignments

6f01ff2

Enforce alignment/indentation on variable assignments that span multiple
lines.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins mentioned this pull request

V5.10.2 proposal #6311

Closed

MylesBorins pushed a commit that referenced this pull request


          lib,test,tools: alignment on variable assignments

c8783af

Correct alignment on variable assignments that span multiple lines in
preparation for lint rule to enforce such alignment.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request


          tools: lint for alignment of variable assignments

be070d7

Enforce alignment/indentation on variable assignments that span multiple
lines.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

This was referenced Apr 21, 2016

V5.11.0 proposal #6321

Closed

V5.11.0 proposal #6322

Closed

MylesBorins pushed a commit that referenced this pull request


          lib,test,tools: alignment on variable assignments

90c12a9

Correct alignment on variable assignments that span multiple lines in
preparation for lint rule to enforce such alignment.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins pushed a commit that referenced this pull request


          tools: lint for alignment of variable assignments

7ea5e43

Enforce alignment/indentation on variable assignments that span multiple
lines.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request


          lib,test,tools: alignment on variable assignments

57da7b9

Correct alignment on variable assignments that span multiple lines in
preparation for lint rule to enforce such alignment.

PR-URL: nodejs#6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

joelostrowski pushed a commit to joelostrowski/node that referenced this pull request


          tools: lint for alignment of variable assignments

f71fb18

Enforce alignment/indentation on variable assignments that span multiple
lines.

PR-URL: nodejs#6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell pushed a commit that referenced this pull request


          lib,test,tools: alignment on variable assignments

6f073ce

Correct alignment on variable assignments that span multiple lines in
preparation for lint rule to enforce such alignment.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell pushed a commit that referenced this pull request


          tools: lint for alignment of variable assignments

d84b00c

Enforce alignment/indentation on variable assignments that span multiple
lines.

PR-URL: #6242
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: James M Snell <jasnell@gmail.com>

mscdex commented Apr 27, 2016

Contributor

@Trott This change is causing linter issues due to the use of the default parameters. For example:

/usr/home/iojs/build/workspace/node-test-linter/tools/eslint-rules/align-multiline-assignment.js:10
function getBinaryExpressionStarts(binaryExpression, starts = []) {
                                                            ^

SyntaxError: Unexpected token =
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:387:25)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at Object.get (/usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/rules.js:70:16)
    at getRuleOptionsSchema (/usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/config/config-validator.js:33:22)
    at validateRuleOptions (/usr/home/iojs/build/workspace/node-test-linter/tools/eslint/lib/config/config-validator.js:70:18)

Trott commented Apr 27, 2016

Member Author

That's weird. Default parameters are in the current node and presumably that CI is using current master with the current version of v8 that we have in master...

Trott commented Apr 27, 2016 •

edited

Loading

Member Author

@mscdex Looks like ~~the~~ a problem might be in the lint-ci job. If I introduce a lint error into the code, make jslint runs fine, but make lint-ci returns no errors and exits with a successful code.

silverwind commented Apr 28, 2016

Contributor

@Trott I also see it locally, even when running eslint directly:

$ node tools/eslint/bin/eslint.js --rulesdir tools/eslint-rules test/doctool
Unexpected token =
SyntaxError: Unexpected token =
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:387:25)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at Object.get (/Users/silverwind/git/node/tools/eslint/lib/rules.js:70:16)
    at getRuleOptionsSchema (/Users/silverwind/git/node/tools/eslint/lib/config/config-validator.js:33:22)
    at validateRuleOptions (/Users/silverwind/git/node/tools/eslint/lib/config/config-validator.js:70:18)

Maybe it's an ESLint bug?

silverwind commented Apr 28, 2016

Contributor

Nevermind, my local node was not 6.0.0 yet, working now.

Trott commented Apr 28, 2016

Member Author

For the record, the issue is that the VM in CI that does linting uses the system Node.js rather than compiling it from source. It does this so that the linting will finish quickly. There's a PR in to back out the default parameter in our custom lint rule. When it lands, this will be a non-issue again.

MylesBorins commented May 17, 2016

Contributor

@Trott this will need to be manually backported

Trott mentioned this pull request

6242 backport #6869

Closed

Trott commented May 18, 2016

Member Author

@thealphanerd Here you go: #6869

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels

Trott deleted the align branch

January 13, 2022 22:43

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

tools