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

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

Why add the _ prefix?

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

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

oups, you're right, thanks

Maybe use a camel case name like readableResume ?

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

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

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

@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

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

@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

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

LGTM if CI is green

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

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

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

Some nits.

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

can you please name this one?

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

can you please name this?

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

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

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

Why not name this?

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

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

A couple of nits and we are good to go.

Can you squash the commits as well?

can this have a different name? Like _hasInstancePolyfill?

can we name this writableHasInstance?

how does it looks now @mcollina?

are you sure you can use this here?

self is not defined anymore in this function.

why this?

can you please revert this to self?

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

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

outdated

@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

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

Can you restore these spaces?

Ref: nodejs#8913

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

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

LGTM if ci is green.

@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

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

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

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

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

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

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

Uh oh!

Uh oh!

stream: name anonymous functions - #9130

stream: name anonymous functions#9130maasencioh wants to merge 2 commits intonodejs:masternodejs/node:masterfrom maasencioh:name_streammaasencioh/node:name_streamCopy head branch name to clipboard

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!

stream: name anonymous functions#9130
maasencioh wants to merge 2 commits into
nodejs:masterfrom
maasencioh:name_stream

maasencioh commented Oct 17, 2016 •
edited
Loading

lpinca commented Oct 17, 2016 •
edited
Loading

maasencioh commented Oct 18, 2016 •
edited
Loading