fs: add bytesRead to ReadStream #7942

LinusU · 2016-08-01T19:43:55Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

Add a property named bytesRead that exposes how many bytes that have currently been read from the file. This brings consistency with WriteStream that has bytesWritten and net.Socket which have both bytesRead and bytesWritten.

Fixes #7938

addaleax · 2016-08-01T19:55:23Z

test/parallel/test-fs-read-stream.js

 callbacks.end++;
 });


 file.on('close', function() {
+ assert.strictEqual(file.bytesRead, bytesRead);


You could also add a line that compares file.bytesRead with the actual file size, like this:

assert.strictEqual(file.bytesRead, fs.statSync(fn).size);

jasnell · 2016-08-01T20:22:20Z

Generally LGTM with Green CI and @addaleax's comment addressed

ronkorving · 2016-08-02T05:50:29Z

Thank you for this, a very welcome addition. 👍

LinusU · 2016-08-02T06:02:56Z

Addressed comment and rebased on latest master 👌

edit: How do I trigger CI? Or is that something that someone from the core team does?

brendanashworth · 2016-08-02T07:23:19Z

Here you go: https://ci.nodejs.org/job/node-test-pull-request/3496/

LinusU · 2016-08-02T07:53:52Z

Finished: SUCCESS

✅ Green CI

jasnell · 2016-08-04T19:16:20Z

@addaleax @bnoordhuis @trevnorris ... would love to get your review on this also!

addaleax · 2016-08-04T21:18:57Z

doc/api/fs.md

@@ -181,6 +181,13 @@ added: v0.1.93
 Emitted when the `ReadStream`'s underlying file descriptor has been closed
 using the `fs.close()` method.

+### readStream.bytesRead
+<!-- YAML
+added: v6.4.0


I’d say with 99 % confidence that the version here is accurate, but ideally you can use added: REPLACEME here so that the correct version will be inserted when the release is made.

oh, good catch... yeah, using REPLACEME is likely better

addaleax · 2016-08-04T21:19:40Z

LGTM with a comment, thanks for the work behind this!

bnoordhuis · 2016-08-05T08:48:41Z

LGTM

Add a property named bytesRead that exposes how many bytes that have currently been read from the file. This brings consistency with WriteStream that has bytesWritten and net.Socket which have both bytesRead and bytesWritten. Fixes nodejs#7938

LinusU · 2016-08-05T08:58:41Z

Switched from v6.4.0 to REAPLCEME
Rebased on latest master

jasnell · 2016-08-05T19:53:22Z

Thank you for bearing with us! :-) One more CI run following the updated commits: https://ci.nodejs.org/job/node-test-pull-request/3544/

jasnell · 2016-08-05T22:33:16Z

There was a build bot failure on freebsd... running again just to make sure it's a one off: https://ci.nodejs.org/job/node-test-pull-request/3546/

jasnell · 2016-08-08T16:26:15Z

hmm... failed on freebsd again... one more try: https://ci.nodejs.org/job/node-test-pull-request/3572/

jasnell · 2016-08-08T17:08:30Z

Failed on Windows this time, unrelated build bot failure tho, at least that's different from last time. getting this landed!

Add a property named bytesRead that exposes how many bytes that have currently been read from the file. This brings consistency with WriteStream that has bytesWritten and net.Socket which have both bytesRead and bytesWritten. Fixes: https://github.com/nodejs/node/issues/#7938 PR-URL: #7942 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell · 2016-08-08T17:45:21Z

Landed in 4a87abb. Thank you!

LinusU · 2016-08-09T08:22:10Z

Awesome 🎉 thank you!

Add a property named bytesRead that exposes how many bytes that have currently been read from the file. This brings consistency with WriteStream that has bytesWritten and net.Socket which have both bytesRead and bytesWritten. Fixes: https://github.com/nodejs/node/issues/#7938 PR-URL: #7942 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942 * repl: The REPL now supports editor mode. (Prince J Wesley) #7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013 Refs: #8020 PR-URL: #8070

Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838 * child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) #7696 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942 * repl: The REPL now supports editor mode. (Prince J Wesley) #7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013 Refs: #8020 PR-URL: #8070

Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) nodejs#7983 and nodejs#7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) nodejs#7811 and nodejs#7838 * child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) nodejs#7696 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) nodejs#7942 * repl: The REPL now supports editor mode. (Prince J Wesley) nodejs#7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) nodejs#8013 Refs: nodejs#8020 PR-URL: nodejs#8070

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 1, 2016

LinusU force-pushed the fs-read-stream-bytes-read branch from 8bcf224 to 6b436d9 Compare August 1, 2016 19:45

addaleax reviewed Aug 1, 2016
View reviewed changes

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 1, 2016

LinusU force-pushed the fs-read-stream-bytes-read branch from 6b436d9 to 5fcd263 Compare August 2, 2016 06:02

addaleax reviewed Aug 4, 2016
View reviewed changes

fs: add bytesRead to ReadStream

460a501

Add a property named bytesRead that exposes how many bytes that have currently been read from the file. This brings consistency with WriteStream that has bytesWritten and net.Socket which have both bytesRead and bytesWritten. Fixes nodejs#7938

LinusU force-pushed the fs-read-stream-bytes-read branch from 5fcd263 to 460a501 Compare August 5, 2016 08:58

jasnell closed this Aug 8, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

LinusU deleted the fs-read-stream-bytes-read branch August 9, 2016 08:22

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: add bytesRead to ReadStream #7942

fs: add bytesRead to ReadStream #7942

LinusU commented Aug 1, 2016

addaleax Aug 1, 2016

jasnell commented Aug 1, 2016

ronkorving commented Aug 2, 2016

LinusU commented Aug 2, 2016 •

edited

Loading

brendanashworth commented Aug 2, 2016

LinusU commented Aug 2, 2016 •

edited

Loading

jasnell commented Aug 4, 2016

addaleax Aug 4, 2016

jasnell Aug 4, 2016

addaleax commented Aug 4, 2016

bnoordhuis commented Aug 5, 2016

LinusU commented Aug 5, 2016

jasnell commented Aug 5, 2016

jasnell commented Aug 5, 2016

jasnell commented Aug 8, 2016

jasnell commented Aug 8, 2016

jasnell commented Aug 8, 2016

LinusU commented Aug 9, 2016

fs: add bytesRead to ReadStream #7942

fs: add bytesRead to ReadStream #7942

Conversation

LinusU commented Aug 1, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax Aug 1, 2016

Choose a reason for hiding this comment

jasnell commented Aug 1, 2016

ronkorving commented Aug 2, 2016

LinusU commented Aug 2, 2016 • edited Loading

brendanashworth commented Aug 2, 2016

LinusU commented Aug 2, 2016 • edited Loading

jasnell commented Aug 4, 2016

addaleax Aug 4, 2016

Choose a reason for hiding this comment

jasnell Aug 4, 2016

Choose a reason for hiding this comment

addaleax commented Aug 4, 2016

bnoordhuis commented Aug 5, 2016

LinusU commented Aug 5, 2016

jasnell commented Aug 5, 2016

jasnell commented Aug 5, 2016

jasnell commented Aug 8, 2016

jasnell commented Aug 8, 2016

jasnell commented Aug 8, 2016

LinusU commented Aug 9, 2016

LinusU commented Aug 2, 2016 •

edited

Loading

LinusU commented Aug 2, 2016 •

edited

Loading