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

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.

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

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.

Ditto

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.

Nit: shorthand syntax

Ditto here and on line 326.

LGTM

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

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

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>

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>

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.

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

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

I'd prefer that we do, yes.

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

@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 83c7a88 Compare 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

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

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

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

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

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

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

Uh oh!

Uh oh!

test,lib,benchmark: match function names - #9113

test,lib,benchmark: match function names#9113Trott wants to merge 2 commits intonodejs:masternodejs/node:masterfrom Trott:func-name-matchingTrott/io.js:func-name-matchingCopy head branch name to clipboard

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!

targos Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

targos Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

targos Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

Trott Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

targos Oct 16, 2016

Choose a reason for hiding this comment

Uh oh!

targos Oct 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

test,lib,benchmark: match function names#9113
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:func-name-matching