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.

Sign up for GitHub

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

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

Closed

Trott wants to merge 2 commits into nodejs:master from Trott:align

Member

Trott commented Apr 16, 2016

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

Member Author

Trott commented Apr 16, 2016

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

Member Author

Trott commented Apr 16, 2016

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 =

Member

bnoordhuis Apr 16, 2016 •

edited

Loading

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

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

Member

bnoordhuis commented Apr 16, 2016

Nice. LGTM.

mscdex reviewed

View reviewed changes

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

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

Contributor

mscdex Apr 16, 2016

s/testDecleration/testDeclaration/

jbergstroem mentioned this pull request

tools: consistency of colon spacing in .eslintrc #6210

Closed

2 tasks

Member

jbergstroem commented Apr 16, 2016

LGTM

1 similar comment

Member

jasnell commented Apr 18, 2016

LGTM

Member

jasnell commented Apr 19, 2016

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Member

jasnell commented Apr 19, 2016

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: James M Snell <[email protected]>

Contributor

mscdex commented Apr 27, 2016

@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)

Member Author

Trott commented Apr 27, 2016

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...

Member Author

Trott commented Apr 27, 2016 •

edited

Loading

@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.

Contributor

silverwind commented Apr 28, 2016

@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?

Contributor

silverwind commented Apr 28, 2016

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

Member Author

Trott commented Apr 28, 2016

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.

Contributor

MylesBorins commented May 17, 2016

@Trott this will need to be manually backported

Trott mentioned this pull request

6242 backport #6869

Closed

Member Author

Trott commented May 18, 2016

@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