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: use shorthand properties in tests #18105

Conversation

tniessen
Copy link
Member

This updates the tests to use shorthand property declarations when all properties can be converted to the shortened notation (with two exceptions).

{ a: a, b: b, ... } becomes { a, b }, but { a: a, b: c, ... } remains { a: a, b: c, ... }.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 11, 2018
@devsnek
Copy link
Member

devsnek commented Jan 11, 2018

but { a: a, b: c, ... } remains { a: a, b: c, ... }.

why not { a, b: c }

@tniessen tniessen changed the title It really looks like named parameters which is nice test: use shorthand properties in tests Jan 11, 2018
@tniessen
Copy link
Member Author

why not { a, b: c }

@devsnek Because that change might be controversial. I assumed changing the notation when all properties can be converted won't be, so I decided to go with that for now. If we get consensus about other cases, I will be happy to change those too.

[{ port: port }, { protocol: 'http:' }],
[{ port: port, hostname: '127.0.0.1' }, { protocol: 'http:' }]
[{ port }, { protocol: 'http:' }],
[{ port, hostname: '127.0.0.1' }, { protocol: 'http:' }]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an exception for the sake of consistency with the line above. Feel free to request changes here (and anywhere else).

@@ -63,7 +62,7 @@ function runTest(highWaterMark, objectMode, produce) {
}

const w = new Writable({ highWaterMark: highWaterMark * 2,
objectMode: objectMode });
objectMode });
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the second exception to the described rule, again, feel free to request changes here.

@starkwang
Copy link
Contributor

Maybe we can add the "object-shorthand" rule with "consistent as needed" option into .eslintrc ?

@joyeecheung
Copy link
Member

@tniessen
Copy link
Member Author

CI seems to be a bit unreliable, trying again: https://ci.nodejs.org/job/node-test-pull-request/12579/

@tniessen
Copy link
Member Author

Landed in db9c556.

@tniessen tniessen closed this Jan 17, 2018
tniessen added a commit that referenced this pull request Jan 17, 2018
PR-URL: #18105
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this pull request Jan 30, 2018
PR-URL: #18105
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
PR-URL: #18105
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants