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

test: adjust indentation for stricter linting #14431

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 23, 2017

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

@Trott 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 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
@@ -62,7 +62,7 @@ if (typeof Symbol !== 'undefined' && 'hasInstance' in Symbol &&

function compareToNative(theObject, theConstructor) {
assert.strictEqual(addon.doInstanceOf(theObject, theConstructor),
Copy link
Contributor

@XadillaX XadillaX Jul 23, 2017

Choose a reason for hiding this comment

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

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(...),
  (theObject instanceof theConstructor));

VS

assert.strictEqual(addon.doInstanceOf(...),
  (theObject instanceof theConstructor));

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

MemberExpression: off,
ObjectExpression: first,
SwitchCase: 1}]
indent-legacy: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

// these lines should contain tab!
// eslint-disable-next-line no-throw-literal
throw ({ foo: 'bar' });
// these lines should contain tab!
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -1,5 +1,5 @@
before
*test*message*throw_in_line_with_tabs.js:*
throw ({ foo: 'bar' });
^
throw ({ foo: 'bar' });
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -31,7 +31,7 @@ deepStrictEqual(Buffer.from(new String(checkString)), check);
deepStrictEqual(Buffer.from(new MyString()), check);
deepStrictEqual(Buffer.from(new MyPrimitive()), check);
deepStrictEqual(Buffer.from(
runInNewContext('new String(checkString)', { checkString })),
runInNewContext('new String(checkString)', { checkString })),
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this line can be unwrapped now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

message: 'The "string" argument must be of type string. ' +
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError,
message: 'The "string" argument must be of type string. ' +
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jul 23, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Some nits

allowNoInit,
logid,
logtype } = {}) {
oninit,
Copy link
Contributor

@refack refack Jul 23, 2017

Choose a reason for hiding this comment

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

I guess this is very C++, but not my cup of tea.
If there are no objections I'd rather have

exports = module.exports = function initHooks(
  { oninit, onbefore, onafter, ondestroy, allowNoInit, logid, logtype } = {}) {

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -129,7 +129,7 @@ function expectBody(expected) {
//
{
const request = Buffer.from(
'HTTP/1.1 200 OK' + CRLF +
'HTTP/1.1 200 OK' + CRLF +
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Trott Trott Jul 24, 2017

Choose a reason for hiding this comment

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

Continuation indentation is ignored. So this is fine by the linter:

  const request = Buffer.from(
    'HTTP/1.1 200 OK' + CRLF +
    'Content-Type: text/plain' + CRLF +
    'Content-Length: 4' + CRLF +
    CRLF +
    'pong'
  );

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

All squared up.

'--use-bundled-ca',
'--use-openssl-ca',
'-p', 'process.version'],
'--use-bundled-ca',
Copy link
Contributor

Choose a reason for hiding this comment

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

could you chop all the parameters:

const result = childProcess.spawnSync(
  process.execPath,
  [
    '--use-bundled-ca',
    '--use-openssl-ca',
    '-p', 'process.version'
  ],
  { encoding: 'utf8' }
);

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like this one:

const result = childProcess.spawnSync(
  process.execPath, [
    '--use-bundled-ca',
    '--use-openssl-ca',
    '-p', 'process.version'
  ], {
    encoding: 'utf8'
  });

Copy link
Contributor

Choose a reason for hiding this comment

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

Either one's good by me,

Copy link
Member Author

Choose a reason for hiding this comment

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

Went with this third option:

const result = childProcess.spawnSync(
  process.execPath,
  [ '--use-bundled-ca', '--use-openssl-ca', '-p', 'process.version' ],
  { encoding: 'utf8' }
);

@refack
Copy link
Contributor

refack commented Jul 23, 2017

@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 Trott force-pushed the indent-tests branch 3 times, most recently from deda5b7 to 63f23d7 Compare July 24, 2017 21:31
@Trott Trott force-pushed the indent-tests branch 3 times, most recently from 20f1051 to ff0c507 Compare July 24, 2017 22:31
@Trott
Copy link
Member Author

Trott commented Jul 24, 2017

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

ondestroy,
allowNoInit,
logid,
logtype
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jul 25, 2017

Choose a reason for hiding this comment

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

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

assert.strictEqual(Buffer.isEncoding(enc), true);
});
assert.strictEqual(Buffer.isEncoding(enc), true);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Will fix.

@@ -92,7 +91,7 @@ function expectBody(expected) {
// Simple request test.
//
{
const request = Buffer.from(`GET /hello HTTP/1.1${CRLF}${CRLF}`);
const request = Buffer.from('GET /hello HTTP/1.1\r\n\r\n');
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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'll move that file's changes into its own commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved to its own commit.

@Trott Trott force-pushed the indent-tests branch 3 times, most recently from 478b3f1 to 2e13dc4 Compare July 25, 2017 17:37
Trott added a commit to Trott/io.js that referenced this pull request Jul 27, 2017
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 <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jul 27, 2017

Landed in 4f0b107 and aa6fac6

@Trott Trott closed this Jul 27, 2017
addaleax pushed a commit that referenced this pull request Jul 27, 2017
* eliminate CRLF identifier, reducing string concatenation
* adjust indentation for stricter ESLint 4.x indentation linting

PR-URL: #14431
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@addaleax
Copy link
Member

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.

Trott added a commit to Trott/io.js that referenced this pull request Jul 27, 2017
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 <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jul 27, 2017

Backported in #14522.

addaleax pushed a commit that referenced this pull request Jul 28, 2017
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 <anna@addaleax.net>

PR-URL: #14431
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@addaleax addaleax mentioned this pull request Aug 2, 2017
Trott added a commit to Trott/io.js that referenced this pull request Aug 15, 2017
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 <anna@addaleax.net>

PR-URL: nodejs#14431
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
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 <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
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 <refack@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@Trott Trott deleted the indent-tests branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. 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. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants