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: option for engine in convenience methods #13089

Closed
wants to merge 3 commits into from
Closed

zlib: option for engine in convenience methods #13089

wants to merge 3 commits into from

Conversation

AlexanderOMara
Copy link
Contributor

@AlexanderOMara AlexanderOMara commented May 18, 2017 •
edited
Loading

Added option to expose engine to convenience methods

Refs: #8874

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
Affected core subsystem(s)
  • zlib

Alternative to Pull Request #12939, broken into 2 separate commits.

@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label May 18, 2017
@mscdex mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 18, 2017
@AlexanderOMara AlexanderOMara mentioned this pull request May 18, 2017
Closed
4 tasks
sam-github
sam-github previously requested changes May 18, 2017
View reviewed changes
Copy link
Contributor

@sam-github sam-github left a comment

Choose a reason for hiding this comment

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

Hi, thanks for this, can you add documentation? In the absence of documentation its difficult to figure out what the code is intended to do, or to review the code to see if its behaving as intended.

@AlexanderOMara AlexanderOMara force-pushed the zlib-convenience-info branch from 4406738 to 2c36616 Compare May 18, 2017 01:14
@AlexanderOMara
zlib: option for engine in convenience methods
e5ca118 
Added option to expose engine to convenience methods

Refs: nodejs#8874
@AlexanderOMara AlexanderOMara force-pushed the zlib-convenience-info branch from 2c36616 to e5ca118 Compare May 18, 2017 01:14
@AlexanderOMara AlexanderOMara changed the title zlib: added option to expose engine to convenience methods zlib: option for engine in convenience methods May 18, 2017
@addaleax
Copy link
Member

@mscdex Why is this semver-major?

@mscdex
Copy link
Contributor

mscdex commented May 18, 2017 •
edited
Loading

@addaleax Because of the possible change in values passed to the callback or returned by the sync function? Previously it was always buffer, but now it could be an object containing buffer. I would bet people would not expect that?

@addaleax
Copy link
Member

@mscdex Yes, but it’s explicitly opt-in. The returned value is an object only when opts.info was set to true, which is a new flag that’s being introduced here (maybe that’s not obvious without docs?).

@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 18, 2017
@mscdex
Copy link
Contributor

mscdex commented May 18, 2017

@addaleax Probably. Docs should be added I think, especially in case someone unknowingly passes an object with info set in it.

@addaleax
Copy link
Member

@AlexanderOMara To be clear, once you add docs here this should be good. 👍

@AlexanderOMara
Copy link
Contributor Author

I've added some documentation. Let me know if it needs any changes.

mscdex
mscdex reviewed May 22, 2017
View reviewed changes
doc/api/zlib.md Outdated
@@ -301,6 +301,7 @@ ignored by the decompression classes.
* `strategy` {integer} (compression only)
* `dictionary` {Buffer|TypedArray|DataView} (deflate/inflate only, empty dictionary by
default)
* `info` {boolean} (It `true`, returns object with buffer and engine)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/It/If/
s/returns object/returns an object/
Also, perhaps put 'buffer' and 'engine' in backticks to give a hint that they are the actual object property names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call! Edits made.

addaleax
addaleax approved these changes May 23, 2017
View reviewed changes
doc/api/zlib.md
@@ -301,6 +301,7 @@ ignored by the decompression classes.
* `strategy` {integer} (compression only)
* `dictionary` {Buffer|TypedArray|DataView} (deflate/inflate only, empty dictionary by
default)
* `info` {boolean} (If `true`, returns an object with `buffer` and `engine`)
Copy link
Member

Choose a reason for hiding this comment

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

nit: I’d drop the parentheses, and maybe add properties at the end to be explicit

@addaleax addaleax dismissed sam-github’s stale review May 24, 2017 10:48

documentation is added

@addaleax
Copy link
Member

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

@addaleax
Copy link
Member

Landed in d0b1b52

@addaleax addaleax closed this May 31, 2017
addaleax pushed a commit that referenced this pull request May 31, 2017
@AlexanderOMara @addaleax
zlib: option for engine in convenience methods
d0b1b52 
Added option to expose engine to convenience methods

Refs: #8874
PR-URL: #13089
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this pull request Jun 5, 2017
@AlexanderOMara @jasnell
zlib: option for engine in convenience methods
bb77d6c 
Added option to expose engine to convenience methods

Refs: #8874
PR-URL: #13089
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell jasnell mentioned this pull request Jun 5, 2017
Closed
@gibfahn
Copy link
Member

gibfahn commented Jan 15, 2018

Release team decided not to backport to v6.x.

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

@mscdex mscdex mscdex left review comments

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@sam-github sam-github sam-github left review comments

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.
7 participants
@AlexanderOMara @addaleax @mscdex @gibfahn @sam-github @jasnell @nodejs-github-bot