test: adjust indentation for stricter linting#14431

Trott · 2017-07-23T00:56:51Z

ESLint 4.x has stricter linting than previous versions. We are currently
using the legacy indentation rules in the test directory. This commit
changes the indentation of files to comply with the stricter 4.x
linting and enable stricter linting in the test directory.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test tools

XadillaX · 2017-07-23T09:01:18Z

test/addons-napi/test_general/testInstanceOf.js

If we have to split parameters into several lines, I'd like to let first parameter use a new line also.
eg.
assert.strictEqual(addon.doInstanceOf(...),(theObjectinstanceoftheConstructor));
VS
assert.strictEqual(addon.doInstanceOf(...),(theObjectinstanceoftheConstructor));

I'll do it here and anywhere else someone flags in this PR, but not sure about everywhere because this sort of thing is all over our code base already and the churn would be extreme.

vsemozhetbyt · 2017-07-23T10:36:41Z

test/.eslintrc.yaml

a tiny nit: may be off according to overall verbose style.

Oh, yes, and I'll make the same change in lib/.eslintrc.yaml too!

vsemozhetbyt · 2017-07-23T10:46:10Z

test/message/throw_in_line_with_tabs.js

I am a bit confused here. The comment states the tabs are needed here (as the eslint comment above disabling no-tabs).

Oops, yeah, I probably messed that test up. Will fix...

vsemozhetbyt · 2017-07-23T10:46:40Z

test/message/throw_in_line_with_tabs.out

vsemozhetbyt · 2017-07-23T11:04:43Z

test/parallel/test-buffer-from.js

It seems this line can be unwrapped now.

vsemozhetbyt · 2017-07-23T11:08:00Z

test/parallel/test-buffer-new.js

It seems this line can be unwrapped (de-concatenated) now.

refack

Some nits

refack · 2017-07-23T21:45:40Z

test/async-hooks/init-hooks.js

I guess this is very C++, but not my cup of tea.
If there are no objections I'd rather have
exports=module.exports=functioninitHooks({ oninit, onbefore, onafter, ondestroy, allowNoInit, logid, logtype }={}){

Uhh I'd prefer the form in the PR, because imagine what that variant would look like if I were to add another option.

Yeah, it's definatly at it's limit... Although at a later stage we could move the destructuring into the function's body...
Anyway, I'm not strongly opposed to current format.

Improved readability slightly by adding a new line after logtype...

refack · 2017-07-23T21:51:10Z

test/parallel/test-http-parser.js

That's the rule? just the first one, and then there's a continuation indent added...
Not amazing 🤷‍♂️

Continuation indentation is ignored. So this is fine by the linter:
constrequest=Buffer.from('HTTP/1.1 200 OK'+CRLF+'Content-Type: text/plain'+CRLF+'Content-Length: 4'+CRLF+CRLF+'pong');

IMHO it should be "square"
AFAICT it's only in this file.

Will square them all up. Might also take the opportunity to get rid of the CRLF constant...

All squared up.

refack · 2017-07-23T21:53:58Z

test/parallel/test-openssl-ca-options.js

could you chop all the parameters:
constresult=childProcess.spawnSync(process.execPath,['--use-bundled-ca','--use-openssl-ca','-p','process.version'],{encoding: 'utf8'});

I'd like this one:
constresult=childProcess.spawnSync(process.execPath,['--use-bundled-ca','--use-openssl-ca','-p','process.version'],{encoding: 'utf8'});

Either one's good by me,

Went with this third option:
constresult=childProcess.spawnSync(process.execPath,['--use-bundled-ca','--use-openssl-ca','-p','process.version'],{encoding: 'utf8'});

refack · 2017-07-23T21:58:44Z

@Trott is an experienced contributor I'm using the red ❌ , but it's not a hard block. I'm assuming this is the result of eslint --fix, cause some fixes are wonky...

Trott · 2017-07-24T22:32:28Z

Nits addressed as far as I know. PTAL, @refack, and anyone else.

trevnorris · 2017-07-25T11:18:16Z

test/async-hooks/init-hooks.js

not related to this PR about whitespace, but could we enforce trailing commas? makes things easier in git when another field is added and only have a 1 line change instead of 2.

I was trying to propose this for the docs, but it seems this lacks approvement:
#12557

trevnorris · 2017-07-25T11:19:55Z

test/parallel/test-buffer-isencoding.js

this just looks weird. having the body of code on the same indentation level is confusing when quickly scanning the code.

Agreed. Will fix.

trevnorris · 2017-07-25T11:22:46Z

test/parallel/test-http-parser.js

This is a non whitespace change in a commit that only refers to changes in whitespace. Please also make note of this is the git message body.

I'll move that file's changes into its own commit.

Moved to its own commit.

ESLint 4.x has stricter linting than previous versions. We are currently using the legacy indentation rules in the test directory. This commit changes the indentation of files to comply with the stricter 4.x linting and enable stricter linting in the test directory. PR-URL: nodejs#14431 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

Trott · 2017-07-27T16:25:30Z

Landed in 4f0b107 and aa6fac6

* eliminate CRLF identifier, reducing string concatenation * adjust indentation for stricter ESLint 4.x indentation linting PR-URL: #14431 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

addaleax · 2017-07-27T22:29:39Z

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it let me know and add the dont-land-on label.

ESLint 4.x has stricter linting than previous versions. We are currently using the legacy indentation rules in the test directory. This commit changes the indentation of files to comply with the stricter 4.x linting and enable stricter linting in the test directory. PR-URL: nodejs#14431 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

Trott · 2017-07-27T23:10:44Z

Backported in #14522.

ESLint 4.x has stricter linting than previous versions. We are currently using the legacy indentation rules in the test directory. This commit changes the indentation of files to comply with the stricter 4.x linting and enable stricter linting in the test directory. Backport-PR-URL: #14522 Backport-Reviewed-By: Anna Henningsen <[email protected]> PR-URL: #14431 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

ESLint 4.x has stricter linting than previous versions. We are currently using the legacy indentation rules in the test directory. This commit changes the indentation of files to comply with the stricter 4.x linting and enable stricter linting in the test directory. Backport-PR-URL: nodejs#14522 Backport-Reviewed-By: Anna Henningsen <[email protected]> PR-URL: nodejs#14431 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

ESLint 4.x has stricter linting than previous versions. We are currently using the legacy indentation rules in the test directory. This commit changes the indentation of files to comply with the stricter 4.x linting and enable stricter linting in the test directory. Backport-PR-URL: #14835 PR-URL: #14431 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Trevor Norris <[email protected]>

Trott added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jul 23, 2017

nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. dont-land-on-v4.x inspector Issues and PRs related to the V8 inspector protocol node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Jul 23, 2017

Trott force-pushed the indent-tests branch from 567a3f4 to 7272586Compare July 23, 2017 00:58

XadillaX reviewed Jul 23, 2017
View reviewed changes

vsemozhetbyt reviewed Jul 23, 2017
View reviewed changes

refack suggested changes Jul 23, 2017
View reviewed changes

Trott force-pushed the indent-tests branch 3 times, most recently from deda5b7 to 63f23d7Compare July 24, 2017 21:31

vsemozhetbyt approved these changes Jul 24, 2017
View reviewed changes

Trott force-pushed the indent-tests branch 3 times, most recently from 20f1051 to ff0c507Compare July 24, 2017 22:31

trevnorris reviewed Jul 25, 2017
View reviewed changes

Trott force-pushed the indent-tests branch 3 times, most recently from 478b3f1 to 2e13dc4Compare July 25, 2017 17:37

Trott closed this Jul 27, 2017

addaleax added the backport-requested-v8.x label Jul 27, 2017

Trott mentioned this pull request Jul 27, 2017
[v8.x backport] test: adjust indentation for stricter linting #14522
Closed
4 tasks

Trott added backported-to-v8.x and removed backport-requested-v8.x labels Jul 27, 2017

refack mentioned this pull request Jul 30, 2017
readline: remove max limit of crlfDelay #13497
Closed
4 tasks

addaleax mentioned this pull request Aug 2, 2017
v8.3.0 proposal #14594
Merged

MylesBorins mentioned this pull request Aug 15, 2017
backport indentation changes nodejs/Release#239
Closed
7 tasks

MylesBorins added the backported-to-v6.x label Aug 16, 2017

MylesBorins mentioned this pull request Aug 16, 2017
v6.11.3-proposal #14852
Merged

Trott deleted the indent-tests branch January 13, 2022 22:46

Uh oh!

test: adjust indentation for stricter linting#14431

test: adjust indentation for stricter linting #14431

Uh oh!

Conversation

Trott commented Jul 23, 2017

Checklist

Affected core subsystem(s)

Uh oh!

XadillaXJul 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytJul 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refackJul 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrottJul 24, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Jul 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbytJul 25, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

XadillaXJul 23, 2017•
edited
Loading

vsemozhetbytJul 23, 2017•
edited
Loading

refackJul 23, 2017•
edited
Loading

TrottJul 24, 2017•
edited
Loading

refack commented Jul 23, 2017•
edited
Loading

vsemozhetbytJul 25, 2017•
edited
Loading