fs: add bytesRead to ReadStream by LinusU · Pull Request #7942 · nodejs/node · GitHub

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

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);

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

Thank you for this, a very welcome addition. 👍

Addressed comment and rebased on latest master 👌

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

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

Finished: SUCCESS

✅ Green CI

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

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

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

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

Switched from v6.4.0 to REAPLCEME
Rebased on latest master

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

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/

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

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>

Landed in 4a87abb. Thank you!

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

fs: add bytesRead to ReadStream by LinusU · Pull Request #7942 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: add bytesRead to ReadStream - #7942

fs: add bytesRead to ReadStream#7942LinusU wants to merge 1 commit intonodejs:masternodejs/node:masterfrom LinusU:fs-read-stream-bytes-readLinusU/node:fs-read-stream-bytes-readCopy head branch name to clipboard

Conversation

LinusU commented Aug 1, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

addaleax Aug 1, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 1, 2016

Uh oh!

ronkorving commented Aug 2, 2016

Uh oh!

LinusU commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brendanashworth commented Aug 2, 2016

Uh oh!

LinusU commented Aug 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 4, 2016

Uh oh!

addaleax Aug 4, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell Aug 4, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Aug 4, 2016

Uh oh!

bnoordhuis commented Aug 5, 2016

Uh oh!

LinusU commented Aug 5, 2016

Uh oh!

jasnell commented Aug 5, 2016

Uh oh!

jasnell commented Aug 5, 2016

Uh oh!

jasnell commented Aug 8, 2016

Uh oh!

jasnell commented Aug 8, 2016

Uh oh!

jasnell commented Aug 8, 2016

Uh oh!

LinusU commented Aug 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fs: add bytesRead to ReadStream#7942
LinusU wants to merge 1 commit into
nodejs:masterfrom
LinusU:fs-read-stream-bytes-read

LinusU commented Aug 2, 2016 •
edited
Loading

LinusU commented Aug 2, 2016 •
edited
Loading