test,lib,benchmark: match function names by Trott · Pull Request #9113 · nodejs/node

Trott · 2016-10-15T22:09:52Z

Checklist

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

Affected core subsystem(s)

test lib benchmark

Description of change

In most cases, named functions match the variable or property to which
they are being assigned. That also seems to be the practice in a series
of PRs currently being evaluated that name currently-anonymous
functions.

This change applies that rule to instances in the code base that don't
comply with that practice.

This will be enforceable with a lint rule once we upgrade to ESLint
3.8.0.

In most cases, named functions match the variable or property to which they are being assigned. That also seems to be the practice in a series of PRs currently being evaluated that name currently-anonymous functions. This change applies that rule to instances in the code base that don't comply with that practice. This will be enforceable with a lint rule once we upgrade to ESLint 3.8.0.

targos

I don't mind the changes. +0 on activating the rule.

targos · 2016-10-16T05:41:53Z

 res.reading = handle.reading;
 Object.defineProperty(handle, 'reading', {
- get: function readingGetter() {
+ get: function get() {


What about using get() { ?

I find that confusing to read. I will probably change my opinion as it becomes more common and I get used to seeing it. If there's a strong preference for it among others, I'll use it. But if left to my own devices, I prefer the explicit key: value format instead.

targos · 2016-10-16T05:42:05Z

 return res.reading;
 },
- set: function readingSetter(value) {
+ set: function set(value) {


targos · 2016-10-16T05:45:53Z

 };

- s.destroy = s.destroySoon = function destroy() {
+ s.destroy = s.destroySoon = function destroySoon() {


This one is ambiguous. I would keep destroy if I had the choice. Did ESLint complain?
Anyway that's fine. It's just a test...

Yeah, ESLint wanted it to be destroySoon. I agree that destroy is probably preferable, I'll switch it back.

targos · 2016-10-16T05:47:28Z

 },
 { // Make sure this is always the last test, since we change os.homedir()
- before: function mockHomedirFailure() {
+ before: function before() {


Nit: shorthand syntax

targos · 2016-10-16T05:48:10Z

 input: putIn,
 output: putIn,
- completer: function completerSyncMode(line) {
+ completer: function completer(line) {


Ditto here and on line 326.

jasnell

LGTM

Trott · 2016-10-18T03:43:43Z

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

Trott · 2016-10-18T23:35:19Z

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

Trott · 2016-10-20T05:04:47Z

One more CI for good measure: https://ci.nodejs.org/job/node-test-commit/5698/

Only red in that one is a build failure on a Raspberry Pi.

In most cases, named functions match the variable or property to which they are being assigned. That also seems to be the practice in a series of PRs currently being evaluated that name currently-anonymous functions. This change applies that rule to instances in the code base that don't comply with that practice. This will be enforceable with a lint rule once we upgrade to ESLint 3.8.0. PR-URL: nodejs#9113 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Trott · 2016-10-20T05:22:26Z

Landed in 68ba9aa

In most cases, named functions match the variable or property to which they are being assigned. That also seems to be the practice in a series of PRs currently being evaluated that name currently-anonymous functions. This change applies that rule to instances in the code base that don't comply with that practice. This will be enforceable with a lint rule once we upgrade to ESLint 3.8.0. PR-URL: #9113 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

MylesBorins · 2016-11-15T18:36:31Z

hey @Trott I've backported this to both v6 and v4 in 7b75cb9 and bd99b2d respectively.

The v4 change had some conflicts, specifically in files where you changes touched stuff that didn't exist in the tree.

I'm under the impression we would want this work backported, please let me know if we should revert.

MylesBorins · 2016-11-22T06:49:54Z

it seems this backport did not happen on v4.x @Trott should we backport?

Trott · 2016-11-22T19:26:37Z

it seems this backport did not happen on v4.x @Trott should we backport?

I'd prefer that we do, yes.

MylesBorins · 2016-12-21T20:02:45Z

@Trott would you be willing to backport to v4.x?

In most cases, named functions match the variable or property to which they are being assigned. That also seems to be the practice in a series of PRs currently being evaluated that name currently-anonymous functions. This change applies that rule to instances in the code base that don't comply with that practice. This will be enforceable with a lint rule once we upgrade to ESLint 3.8.0. PR-URL: nodejs#9113 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Trott · 2016-12-21T22:15:42Z

@thealphanerd #10398

In most cases, named functions match the variable or property to which they are being assigned. That also seems to be the practice in a series of PRs currently being evaluated that name currently-anonymous functions. This change applies that rule to instances in the code base that don't comply with that practice. This will be enforceable with a lint rule once we upgrade to ESLint 3.8.0. PR-URL: #9113 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 15, 2016

Trott added test Issues and PRs related to the tests. benchmark Issues and PRs related to the benchmark subsystem. labels Oct 15, 2016

targos reviewed Oct 16, 2016
View reviewed changes

fhinkel approved these changes Oct 16, 2016
View reviewed changes

jasnell approved these changes Oct 17, 2016
View reviewed changes

lpinca mentioned this pull request Oct 17, 2016
stream: name anonymous functions #9130
Closed
2 tasks

squash
1fc5b40

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88Compare October 18, 2016 17:02

Trott closed this Oct 20, 2016

MylesBorins added land-on-v4.x labels Nov 15, 2016

MylesBorins added lts-watch-v4.x and removed land-on-v4.x labels Nov 22, 2016

MylesBorins mentioned this pull request Nov 22, 2016
v6.9.2 proposal #9735
Merged

Trott mentioned this pull request Dec 21, 2016
v4.x backport for #9113: test,lib,benchmark: match function names #10398
Closed
4 tasks

MylesBorins mentioned this pull request Jan 24, 2017
v4.8.0 proposal #10973
Merged

Trott deleted the func-name-matching branch January 13, 2022 22:44

Uh oh!

Uh oh!

Conversation

Trott commented Oct 15, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

targosOct 16, 2016

Choose a reason for hiding this comment

Uh oh!

TrottOct 18, 2016

Choose a reason for hiding this comment

Uh oh!

targosOct 16, 2016

Choose a reason for hiding this comment

Uh oh!

targosOct 16, 2016

Choose a reason for hiding this comment

Uh oh!

TrottOct 18, 2016

Choose a reason for hiding this comment

Uh oh!

targosOct 16, 2016

Choose a reason for hiding this comment

Uh oh!

targosOct 16, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Oct 18, 2016

Uh oh!

Trott commented Oct 18, 2016

Uh oh!

Trott commented Oct 20, 2016

Uh oh!

Trott commented Oct 20, 2016

Uh oh!

MylesBorins commented Nov 15, 2016

Uh oh!

MylesBorins commented Nov 22, 2016

Uh oh!

Trott commented Nov 22, 2016

Uh oh!

MylesBorins commented Dec 21, 2016

Uh oh!

Trott commented Dec 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants