stream: name anonymous functions by maasencioh · Pull Request #9130 · nodejs/node

maasencioh · 2016-10-17T11:53:29Z

Checklist

make -j8 test
commit message follows commit guidelines

Description of change

For some callbacks I made a change to arrow funtions and for _stream_readable there was a change in Readable.prototype.wrap that I would like some opinions because I'm not sure if it's going to work as spected

jasnell · 2016-10-17T16:29:42Z

lib/_stream_transform.js

Why add the _ prefix?

This is to avoid conflict with afterTransform that is called on next line.

jasnell · 2016-10-17T16:30:23Z

lib/_stream_wrap.js

since these are being changed to arrow functions, the self can be switched to this

oups, you're right, thanks

targos · 2016-10-17T17:18:35Z

lib/_stream_readable.js

Maybe use a camel case name like readableResume ?

I was thinking to change it to _resume, in order to be consistent

targos · 2016-10-17T17:22:41Z

lib/_stream_transform.js

This is to avoid conflict with afterTransform that is called on next line.

lpinca · 2016-10-17T19:44:05Z

I wonder if some of these changes, like _afterTransform, conflict with #9113.

maasencioh · 2016-10-17T19:53:10Z

@lpinca I just look at those and we both change _stream_wrap.js but in different lines, so it shouldn't conflict, but thank for pointing it out, I didn't review other PRs

lpinca · 2016-10-17T19:57:26Z

@maasencioh my bad, I thought #9113 was proposing an eslint rule to check if names matched. Ignore me.

maasencioh · 2016-10-18T13:33:11Z

@jasnell not all the self were removed, because in some cases the arrow funtion was inside a regular function, I run the tests and everithing it's currently passing

targos · 2016-10-24T08:44:13Z

lib/_stream_readable.js

Maybe call this one resume and rename the local function to resumeReadable ?

jasnell

LGTM if CI is green

targos · 2016-10-24T14:36:48Z

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

maasencioh · 2016-11-03T09:27:55Z

I'm not sure that the CI failures are related to the changes

maasencioh · 2016-11-08T08:39:08Z

@jasnell sorry it's anything else needed in this PR?

mcollina

Some nits.

mcollina · 2017-01-17T10:24:43Z

lib/_stream_wrap.js

Can you please revert all of those to function and name them?

mcollina · 2017-01-17T10:24:56Z

lib/_stream_wrap.js

can you please name this one?

mcollina · 2017-01-17T10:25:43Z

lib/_stream_wrap.js

can you please convert this forEach in a for loop, it can probably save us a couple of ticks.

mcollina · 2017-01-17T10:25:56Z

lib/_stream_wrap.js

can you please name this?

mcollina · 2017-01-17T10:26:43Z

lib/_stream_wrap.js

this should be named as well, also can you make this function top level, so that the for loop under can be optimized.

mcollina · 2017-01-26T10:25:27Z

@maasencioh would you mind addressing the comments above? We can land this!

maasencioh · 2017-01-29T00:57:37Z

Hello @mcollina, I thought that this was forgotten, but here I just added the new nits

cjihrig · 2017-01-29T01:30:02Z

lib/_stream_readable.js

Why not name this?

sorry I miss that one, I'll check on the other ones

cjihrig · 2017-01-29T01:32:00Z

lib/_stream_readable.js

Feel free to ignore, but I prefer naming handlers like onEnd, onData, etc.

I wasn't sure which name could be more useful, so this comments are great, thanks

mcollina

A couple of nits and we are good to go.

Can you squash the commits as well?

mcollina · 2017-01-29T11:25:50Z

lib/_stream_writable.js

can this have a different name? Like _hasInstancePolyfill?

mcollina · 2017-01-29T11:26:09Z

lib/_stream_writable.js

can we name this writableHasInstance?

maasencioh · 2017-01-30T00:34:51Z

how does it looks now @mcollina?

mcollina · 2017-01-30T07:10:42Z

lib/_stream_wrap.js

are you sure you can use this here?

mcollina · 2017-01-30T07:11:02Z

lib/_stream_wrap.js

self is not defined anymore in this function.

mcollina · 2017-01-30T07:11:46Z

lib/_stream_wrap.js

why this?

mcollina · 2017-01-30T07:12:26Z

lib/_stream_wrap.js

can you please revert this to self?

mcollina · 2017-01-30T07:30:14Z

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

mcollina · 2017-01-30T07:31:13Z

@maasencioh can you swash all the streams commit into one? I think it'd be easier to move around.

outdated

maasencioh · 2017-02-01T23:31:11Z

@mcollina I took my time to make it completely like it should be, I changed everything, but I hope that all it's correct now

TimothyGu · 2017-02-01T23:34:53Z

lib/_stream_readable.js

I think the norm is to omit the space after the function name.

@TimothyGu any tip to run the linter and test only on this files? thanks

TimothyGu · 2017-02-01T23:36:28Z

lib/_stream_readable.js

Can you restore these spaces?

Ref: nodejs#8913

maasencioh · 2017-02-20T15:14:03Z

@mcollina what do you think now? any tip to run the linter and test only on this files? thanks

targos · 2017-02-21T07:13:11Z

@maasencioh run make jslint to run the linter. It will check all files the first time but the results are cached so subsequent runs should only check modified files.

mcollina · 2017-02-24T17:39:04Z

LGTM if ci is green.

maasencioh · 2017-02-24T23:59:55Z

@jasnell @mcollina @targos could you please help me whith this doubt?

I renamed a function as proposed like this:

Object.defineProperty(Writable, Symbol.hasInstance, {
  value: function writableHasInstance(object) {
    if (realHasInstance.call(this, object))
      return true;

    return object && object._writableState instanceof WritableState;
  }
});

But currently this throws a jslint error

137:5  error  Function name `writableHasInstance` should match property name `value`             func-name-matching

Should I follow the func-name-matching rule? because this will force me to rename several functions to avoid conflicts

mcollina · 2017-02-25T09:44:54Z

@maasencioh I think you can go ahead and leave that function anonymous.

TimothyGu · 2017-02-25T10:21:00Z

@maasencioh, alternatively, you can use an ESLint annotation to suppress that error. See this example in lib/internal/url.js.

fhinkel · 2017-05-26T19:12:39Z

There hasn't been any activity here. I'm closing this. Feel free to reopen (or ping a collaborator) if I closed this in error.

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

mscdex added stream Issues and PRs related to the stream subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Oct 17, 2016

jasnell reviewed Oct 17, 2016

View reviewed changes

targos reviewed Oct 17, 2016

View reviewed changes

maasencioh force-pushed the name_stream branch 3 times, most recently from 2d4565b to 5a04861 Compare October 18, 2016 13:31

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

targos reviewed Oct 24, 2016

View reviewed changes

lib/_stream_readable.js Outdated

Copy link

Member

targos Oct 24, 2016

Choose a reason for hiding this comment

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

Maybe call this one resume and rename the local function to resumeReadable ?

maasencioh force-pushed the name_stream branch from 5a04861 to 3914124 Compare October 24, 2016 08:55

jasnell approved these changes Oct 24, 2016

View reviewed changes

targos previously approved these changes Oct 24, 2016

View reviewed changes

mcollina requested changes Jan 17, 2017

View reviewed changes

mcollina added the stalled Issues and PRs that are stalled. label Jan 26, 2017

cjihrig reviewed Jan 29, 2017

View reviewed changes

mcollina removed the stalled Issues and PRs that are stalled. label Jan 29, 2017

mcollina requested changes Jan 29, 2017

View reviewed changes

maasencioh force-pushed the name_stream branch from 1372b18 to 3b05cc4 Compare January 30, 2017 00:30

mcollina reviewed Jan 30, 2017

View reviewed changes

maasencioh force-pushed the name_stream branch from 3b05cc4 to ca69508 Compare February 1, 2017 23:26

TimothyGu reviewed Feb 1, 2017

View reviewed changes

lib/_stream_readable.js Outdated

Copy link

Member

TimothyGu Feb 1, 2017

Choose a reason for hiding this comment

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

Can you restore these spaces?

maasencioh force-pushed the name_stream branch from b930d7b to c9d14dd Compare February 3, 2017 01:40

stream: name anonymous functions

b7dc912

Ref: nodejs#8913

maasencioh force-pushed the name_stream branch from a4307a1 to b7dc912 Compare February 3, 2017 01:53

stream: name anonymous functions

b5bc041

mcollina approved these changes Feb 24, 2017

View reviewed changes

cjihrig approved these changes Feb 24, 2017

View reviewed changes

jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

fhinkel closed this May 26, 2017

Uh oh!

Conversation

maasencioh commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maasencioh commented Oct 17, 2016

Uh oh!

lpinca commented Oct 17, 2016

Uh oh!

maasencioh commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Oct 24, 2016

Uh oh!

maasencioh commented Nov 3, 2016

Uh oh!

maasencioh commented Nov 8, 2016

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Jan 26, 2017

Uh oh!

maasencioh commented Jan 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maasencioh commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maasencioh commented Oct 17, 2016 •

edited

Loading

lpinca commented Oct 17, 2016 •

edited

Loading

maasencioh commented Oct 18, 2016 •

edited

Loading