Skip to content
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.

Sign up for GitHub

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

Jump to bottom

zlib: use .bytesWritten instead of .bytesRead #19414

Closed
wants to merge 3 commits into from
Closed

zlib: use .bytesWritten instead of .bytesRead #19414

wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

The introduction of .bytesRead to zlib streams was unfortunate,
because other streams in Node.js core use the exact opposite naming
of .bytesRead and .bytesWritten.

While one could see how the original naming makes sense in
a Transform stream context, we should try to work towards more
consistent APIs in core for these things.

This introduces zlib.bytesWritten and documentation-only deprecates
zlib.bytesRead.

(Luckily, the old property name hasn’t even been around for a whole
year yet.)

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

@addaleax addaleax added zlib Issues and PRs related to the zlib subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 17, 2018
@addaleax addaleax requested a review from a team March 17, 2018 16:07
@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Mar 17, 2018
@addaleax
Copy link
Member Author

addaleax commented Mar 17, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-commit/16972/
CI: https://ci.nodejs.org/job/node-test-commit/16973

@addaleax addaleax force-pushed the zlib-bytes-written branch from 3a5754d to a9427f1 Compare March 17, 2018 16:11
mcollina
mcollina approved these changes Mar 17, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM.

ChALkeR
ChALkeR reviewed Mar 17, 2018
View reviewed changes
lib/zlib.js Outdated
@@ -548,8 +563,8 @@ function processCallback() {
var availOutAfter = state[0];
var availInAfter = state[1];

var inDelta = (handle.availInBefore - availInAfter);
self.bytesRead += inDelta;
var inDelta = handle.availInBefore - availInAfter;
Copy link
Member

Choose a reason for hiding this comment

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

const, if this line is changed to fix style?

vsemozhetbyt
vsemozhetbyt reviewed Mar 18, 2018
View reviewed changes
doc/api/zlib.md Outdated
@@ -766,4 +781,5 @@ Decompress a chunk of data with [Unzip][].
[Unzip]: #zlib_class_zlib_unzip
[`UV_THREADPOOL_SIZE`]: cli.html#cli_uv_threadpool_size_size
[options]: #zlib_class_options
[`zlib.bytesWritten`]: #zlib_zlib_byteswritten
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: it seems to be out of ASCII order (if we still follow this order in references).

addaleax added 3 commits March 29, 2018 23:18
@addaleax
zlib: use .bytesWritten instead of .bytesRead
ef34910 
The introduction of `.bytesRead` to zlib streams was unfortunate,
because other streams in Node.js core use the exact opposite naming
of `.bytesRead` and `.bytesWritten`.

While one could see how the original naming makes sense in
a `Transform` stream context, we should try to work towards more
consistent APIs in core for these things.

This introduces `zlib.bytesWritten` and documentation-only deprecates
`zlib.bytesRead`.

(Luckily, the old property name hasn’t even been around for a whole
year yet.)

Refs: nodejs#8874
Refs: nodejs#13088
@addaleax
[squash] var → const
eac6460
@addaleax
[squash] vsemozhetbyt nit
bf5e4ee
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2018
@addaleax addaleax force-pushed the zlib-bytes-written branch from d460811 to bf5e4ee Compare March 29, 2018 21:18
@addaleax
Copy link
Member Author

Rebased

CI: https://ci.nodejs.org/job/node-test-commit/17270/

@addaleax
Copy link
Member Author

Re-runs: https://ci.nodejs.org/job/node-test-commit/17287/

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 9, 2018
@addaleax @BridgeAR
zlib: use .bytesWritten instead of .bytesRead
49fd9c6 
The introduction of `.bytesRead` to zlib streams was unfortunate,
because other streams in Node.js core use the exact opposite naming
of `.bytesRead` and `.bytesWritten`.

While one could see how the original naming makes sense in
a `Transform` stream context, we should try to work towards more
consistent APIs in core for these things.

This introduces `zlib.bytesWritten` and documentation-only deprecates
`zlib.bytesRead`.

PR-URL: nodejs#19414
Refs: nodejs#8874
Refs: nodejs#13088
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

BridgeAR commented Apr 9, 2018

Landed in 49fd9c6 🎉

@BridgeAR BridgeAR closed this Apr 9, 2018
targos pushed a commit that referenced this pull request Apr 12, 2018
@addaleax @targos
zlib: use .bytesWritten instead of .bytesRead
307ce80 
The introduction of `.bytesRead` to zlib streams was unfortunate,
because other streams in Node.js core use the exact opposite naming
of `.bytesRead` and `.bytesWritten`.

While one could see how the original naming makes sense in
a `Transform` stream context, we should try to work towards more
consistent APIs in core for these things.

This introduces `zlib.bytesWritten` and documentation-only deprecates
`zlib.bytesRead`.

PR-URL: #19414
Refs: #8874
Refs: #13088
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax addaleax deleted the zlib-bytes-written branch April 14, 2018 16:19
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 14, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
@addaleax @BridgeAR
zlib: use .bytesWritten instead of .bytesRead
d59398f 
The introduction of `.bytesRead` to zlib streams was unfortunate,
because other streams in Node.js core use the exact opposite naming
of `.bytesRead` and `.bytesWritten`.

While one could see how the original naming makes sense in
a `Transform` stream context, we should try to work towards more
consistent APIs in core for these things.

This introduces `zlib.bytesWritten` and documentation-only deprecates
`zlib.bytesRead`.

PR-URL: nodejs#19414
Refs: nodejs#8874
Refs: nodejs#13088
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

opting to not backport to v8.x, please lmk if this is a mistake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
semver-minor PRs that contain new features and should be released in the next minor version. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@addaleax @BridgeAR @MylesBorins @mcollina @ChALkeR @jasnell @thefourtheye @cjihrig @vsemozhetbyt @nodejs-github-bot