★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/18105Nouvelle recherche | Portail wanayoo
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

Copy link
Member

@tniessen tniessen commented Jan 11, 2018

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 label Jan 11, 2018
@devsnek
Copy link
Member

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

@tniessen tniessen commented Jan 11, 2018

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

@tniessen tniessen Jan 11, 2018

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

@tniessen tniessen Jan 11, 2018

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

@starkwang starkwang commented Jan 13, 2018

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

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 17, 2018

@tniessen
Copy link
Member Author

@tniessen tniessen commented Jan 17, 2018

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

@tniessen
Copy link
Member Author

@tniessen tniessen commented Jan 17, 2018

Landed in db9c556.

@tniessen tniessen closed this Jan 17, 2018
tniessen added a commit that referenced this issue Jan 17, 2018
PR-URL: #18105
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this issue Jan 30, 2018
PR-URL: #18105
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Feb 27, 2018
PR-URL: #18105
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants