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

'data' argument on callback of Transform._flush() #3708

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@piranna
Contributor

piranna commented Nov 7, 2015

This fixes issue #3707

@JungMinu

This comment has been minimized.

Member

JungMinu commented Nov 8, 2015

@piranna If you are submitting a pull request that adds new functionality, please include one or more tests for the new functionality.

@mscdex mscdex added the stream label Nov 8, 2015

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Nov 8, 2015

@domenic

This comment has been minimized.

Member

domenic commented Nov 8, 2015

-1 personally, at least until the use case is explained. The linked issue does not give any reason why this would be a good API change.

@calvinmetcalf

This comment has been minimized.

Member

calvinmetcalf commented Nov 8, 2015

@domenic consistency it's a bit of a footgun that the callback takes the second argument one place and not another, I'm frankly surprised I haven't run into it myself, does need a test though

@piranna take a look at the tests marked stream in this folder which would be where a test for this would go.

@piranna

This comment has been minimized.

Contributor

piranna commented Nov 8, 2015

As @calvinmetcalf says, consistency is the main reason. Yesterday I was doing a Transform and found that _transform() accept to give an optional chunk but _flush() don't, requiring to use push() if you need to send one last data chunk.

I'm currently connected on my cell phone, I'll do the tests tomorrow when I get home.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Nov 8, 2015

Ah, ignore my previous comment, I see it now.
Still, I don't see why would flush method need to pass data. Do you have a use-case?

@piranna

This comment has been minimized.

Contributor

piranna commented Nov 8, 2015

Still, I don't see why would flush method need to pass data.

As said before, consistency. This transform do nothing for each chunk, but the docs says you can pass data for it on this callback inside _transform(), so someone would think intuitively that it's also possible on this callback inside _flush() but the fact is that's currently not possible. Why I need to call to push() on _flush() but not on _transform()? Don't make sense...

@jasnell

This comment has been minimized.

Member

jasnell commented Nov 14, 2015

Hmm.. technically there's nothing wrong with the change and I'm not going to give it a -1, but I'm in agreement that consistency in this case does not seem to be a strong enough argument. As far as I can tell, the reason _flush does not take a data parameter is because there's no actual use case justifying passing or using that data. It likely doesn't hurt to have it there, but would anyone actually use it?

@piranna

This comment has been minimized.

Contributor

piranna commented Nov 14, 2015

It likely doesn't hurt to have it there, but would anyone actually use it?

Well, I was going to because intuitively it should work, until I get to the fact that it was not implemented... :-/

(sorry for not adding the tests yet, I'm really busy lately, but didn't forgot about this)

@jasnell

This comment has been minimized.

Member

jasnell commented Nov 14, 2015

OK that's fair :) out of curiosity, are you able to share the details of
the use case?
On Nov 14, 2015 9:59 AM, "Jesús Leganés Combarro" notifications@github.com
wrote:

It likely doesn't hurt to have it there, but would anyone actually use it?

Well, I was going to because intuitively it should work, until I get to
the fact that it was not implemented... :-/

(sorry for not adding the tests yet, I'm really busy lately, but didn't
forgot about this)


Reply to this email directly or view it on GitHub
#3708 (comment).

@piranna

This comment has been minimized.

Contributor

piranna commented Nov 14, 2015

out of curiosity, are you able to share the details of the use case?

Yes, as you can see these two lines would be implemented as only one. Not too much an issue, but you would expect that's possible to do...

@jasnell

This comment has been minimized.

Member

jasnell commented Nov 15, 2015

Ok. I don't have time to do a technical review at the moment, but could I ask that you please take a moment to expand on the reasons for the change in the commit log message itself? It would be helpful. Thank you!

@calvinmetcalf

This comment has been minimized.

Member

calvinmetcalf commented Nov 15, 2015

see here for the commit message format

@Trott Trott force-pushed the nodejs:master branch to 082cc8d Dec 27, 2015

@jasnell jasnell added the stalled label Mar 22, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Mar 22, 2016

@piranna ... still interested in pursuing this?

@piranna

This comment has been minimized.

Contributor

piranna commented Mar 22, 2016

Hi @jasnell, I'm still interested on this, only that I got attention on other issues. I'll try to do the pull-request this afternoon.

@piranna piranna force-pushed the piranna:patch-1 branch Mar 22, 2016

@piranna

This comment has been minimized.

Contributor

piranna commented Mar 22, 2016

I've just added the missing test and also added a reference to the new data argument on the docs.

@jasnell

This comment has been minimized.

Member

jasnell commented Mar 22, 2016

@nodejs/ctc ... thoughts?

@trevnorris

View changes

test/parallel/test-stream-transform-flush-data.js Outdated
t.end(Buffer.from('blerg'));
t.on('data', function(data)
{

This comment has been minimized.

@trevnorris

trevnorris Mar 23, 2016

Contributor

mind not dropping the { to the next line?

This comment has been minimized.

@piranna

piranna Mar 23, 2016

Contributor

Sorry, I'm used to Allman style here :-P Fixed :-)

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Mar 23, 2016

Adding for API consistency sounds good. Besides the comment I'd also like the commits squashed and a description of the issue in the commit message body. Then add the metadata:

Fixes: https://github.com/nodejs/node/issues/3707

at the bottom of the message body.

Those things addressed, LGTM

@piranna piranna force-pushed the piranna:patch-1 branch Mar 23, 2016

@piranna

This comment has been minimized.

Contributor

piranna commented Mar 23, 2016

Commits squashed, and description and commit metadata added :-)

@jasnell

View changes

test/parallel/test-stream-transform-flush-data.js Outdated
@@ -0,0 +1,24 @@
'use strict';
var assert = require('assert');
var Transform = require('stream').Transform;

This comment has been minimized.

@jasnell

jasnell Mar 23, 2016

Member

please add const common = require('../common') as the first require, and make these const

This comment has been minimized.

@piranna

piranna Mar 23, 2016

Contributor

Done... although common is not used anywhere :-/

@jasnell

View changes

test/parallel/test-stream-transform-flush-data.js Outdated
});
t.end(Buffer.from('blerg'));
t.on('data', function(data) {

This comment has been minimized.

@jasnell

jasnell Mar 23, 2016

Member
t.on('data', common.mustCall((data) => {
  assert.strictEqual(data.toString(), expected);
}));

This comment has been minimized.

@piranna

piranna Mar 23, 2016

Contributor

Done.

@jasnell

This comment has been minimized.

Member

jasnell commented Mar 23, 2016

Left some comments. With those addressed LGTM

@piranna

This comment has been minimized.

Contributor

piranna commented May 26, 2016

Ok, thank you for the info :-)
El 26/5/2016 12:17, "Trevor Norris" notifications@github.com escribió:

Still LGTM. If no one else does, I'll land it tomorrow.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#3708 (comment)

@jasnell

This comment has been minimized.

Member

jasnell commented May 26, 2016

Still LGTM

@mafintosh

This comment has been minimized.

Member

mafintosh commented May 26, 2016

Ok from me on this. Technically this might break some applications that were relying on the 2nd argument being ignored (i know i've written quick-n-dirty code that's been relying on this).

@mafintosh

This comment has been minimized.

Member

mafintosh commented May 26, 2016

@calvinmetcalf we might consider o/ a breaking change in readable-stream if this is merged and requiring a major bump

@piranna

This comment has been minimized.

Contributor

piranna commented May 26, 2016

Ok from me on this. Technically this might break some applications that
were relying on the 2nd argument being ignored (i know i've written
quick-n-dirty code that's been relying on this).

I'm curious about that use case. Are you talking about apps that are
checking the number of arguments that the callback have?

@mafintosh

This comment has been minimized.

Member

mafintosh commented May 26, 2016

@piranna i'm talking about code that used to do something like this

stream._flush = function (callback) {
  fs.write(this.fd, someBuffer, someOffset, someLength, somePosition, callback)
})

fs.write calls the callback with (err, bytesWritten, buffer). with this pr it'll add bytesWritten to the readable end of the stream where as before it was ignored.

@piranna

This comment has been minimized.

Contributor

piranna commented May 26, 2016

Good catch @mafintosh. Then definitely, it should be a major.

@jasnell jasnell added semver-major and removed semver-minor labels May 27, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented May 27, 2016

Switching the major to be extra cautious. We can discuss further if necessary

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Jun 5, 2016

@jasnell Can it land now despite this?

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 5, 2016

If @nodejs/streams folks are good with it

@mcollina

This comment has been minimized.

Member

mcollina commented Jun 6, 2016

Commit message is missing the 'stream:' prefix.

LGTM

@piranna

This comment has been minimized.

Contributor

piranna commented Jun 6, 2016

Must I add it?
El 6/6/2016 8:47, "Matteo Collina" notifications@github.com escribió:

Commit message is missing the 'stream:' prefix.

LGTM


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3708 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAgfvqUJuvg7Qb3Ta3CP55V9szBq9T2Iks5qI8JwgaJpZM4GeBEx
.

@mcollina

This comment has been minimized.

Member

mcollina commented Jun 6, 2016

@piranna yes please

@mcollina

This comment has been minimized.

@piranna piranna force-pushed the piranna:patch-1 branch Jun 7, 2016

@piranna

This comment has been minimized.

Contributor

piranna commented Jun 7, 2016

@piranna yes please

Done. I've seen that there are conflicts, should I rebase with master too?

@mcollina

This comment has been minimized.

Member

mcollina commented Jun 7, 2016

Great and yes :).
I'll merge tomorrow (I'm in EU), if nobody objects.

stream: 'data' argument on callback of Transform._flush()
Add a `data` argument on Transform._flush() callback to be API consistent with Transform._transform().

Fixes: #3707

@piranna piranna force-pushed the piranna:patch-1 branch to 1f238f6 Jun 7, 2016

@piranna

This comment has been minimized.

Contributor

piranna commented Jun 7, 2016

Great and yes :).

Done :-)

I'll merge tomorrow (I'm in EU), if nobody objects.

Me too! I'm from Spain :-) Are you from Italy, maybe? ;-)

@mcollina

This comment has been minimized.

Member

mcollina commented Jun 8, 2016

@piranna I'm merging now, but I see deps/npm/node_modules/npm-registry-client/node_modules/concat-stream/node_modules/readable-stream/doc/stream.markdown
changed.

If it's ok for you, I'll just avoid that change in this case.


--- a/deps/npm/node_modules/npm-registry-client/node_modules/concat-stream/node_modules/readable-stream/doc/stream.markdown
+++ b/deps/npm/node_modules/npm-registry-client/node_modules/concat-stream/node_modules/readable-stream/doc/stream.markdown
@@ -1146,7 +1146,7 @@ has been called.
 #### transform.\_flush(callback)

 * `callback` {Function} Call this function (optionally with an error
-  argument) when you are done flushing any remaining data.
+  argument and data) when you are done flushing any remaining data.
@piranna

This comment has been minimized.

Contributor

piranna commented Jun 8, 2016

As you see it fits, I did that change to update the API docs, no more.
El 8/6/2016 8:55, "Matteo Collina" notifications@github.com escribió:

@piranna https://github.com/piranna I'm merging now, but I see
deps/npm/node_modules/npm-registry-client/node_modules/concat-stream/node_modules/readable-stream/doc/stream.markdown
changed.

If it's ok for you, I'll just avoid that change in this case.

--- a/deps/npm/node_modules/npm-registry-client/node_modules/concat-stream/node_modules/readable-stream/doc/stream.markdown
+++ b/deps/npm/node_modules/npm-registry-client/node_modules/concat-stream/node_modules/readable-stream/doc/stream.markdown
@@ -1146,7 +1146,7 @@ has been called.

transform._flush(callback)

  • callback {Function} Call this function (optionally with an error
    • argument) when you are done flushing any remaining data.
    • argument and data) when you are done flushing any remaining data.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3708 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAgfvv2k-iagAhVAwViCxOSoTNIVMwpjks5qJmdRgaJpZM4GeBEx
.

@mcollina

This comment has been minimized.

Member

mcollina commented Jun 8, 2016

I don't think that's the right file to change, https://github.com/nodejs/node/blob/master/doc/api/stream.md is.

@mcollina

This comment has been minimized.

Member

mcollina commented Jun 9, 2016

Landed as 0cd0118.

@mcollina mcollina closed this Jun 9, 2016

mcollina added a commit that referenced this pull request Jun 9, 2016

stream: 'data' argument on callback of Transform._flush()
Add a `data` argument on Transform._flush() callback to be API
consistent with Transform._transform().

Fixes: #3707
PR-URL: #3708
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment