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

zlib: refactor constants for consistency #7203

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@jasnell
Member

jasnell commented Jun 7, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

zlib

Description of change

refactor zlib constants for consistency with the changes to
constants in fs, os etc. Adds a new zlib.constants, docs-only
deprecation
for the constants hanging directly off zlib, moves
the constants decls from node_zlib.cc into node_constants.cc, and
removes a couple of constants from the docs that apparently no longer
exist in the code any where (surprise!)

/cc @addaleax

@addaleax

View changes

doc/api/zlib.md Outdated
@@ -123,7 +123,7 @@ method that is used to compressed the last chunk of input data:
// This is a truncated version of the buffer from the above examples
const buffer = Buffer.from('eJzT0yMA', 'base64');
zlib.unzip(buffer, { finishFlush: zlib.Z_SYNC_FLUSH }, (err, buffer) => {
zlib.unzip(buffer, {finishFlush: zlib.constantsZ_SYNC_FLUSH}, (err, buffer) => {

This comment has been minimized.

@addaleax

addaleax Jun 8, 2016

Member

typo: missing .

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 8, 2016

@addaleax ... nit fixed!

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 8, 2016

@nodejs/ctc ... please review. This includes a docs only deprecation.

@rvagg

This comment has been minimized.

Member

rvagg commented Jun 8, 2016

👍 for consistency, lgtm

commit message could be more clear about what's going on to make it easier to spot when scanning the changelog

@mscdex

This comment has been minimized.

Contributor

mscdex commented Jun 8, 2016

If this is a docs-only deprecation, why is this a semver-major?

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 8, 2016

@mscdex ... we've generally treated docs-only deprecations as semver-major. In the previous update for require('constants'), we did it as semver-minor because the constants module wasn't actually documented. In this case, the zlib.md actually already documented the constants.

zlib: move constants into zlib.constants
zlib constants were previously being added to binding in node_zlib.cc.
This moves the zlib constants to node_constants.cc for consistency with
the recent constants refactoring:
  #6534

Adds require('zlib').constants to expose the constants
Docs-only deprecates the constants hung directly off require('zlib')
Removes a couple constants from the docs that apparently no longer
exist in the code

@jasnell jasnell force-pushed the jasnell:zlib_constants branch to c418104 Jun 8, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 8, 2016

@rvagg: commit log updated.

CI: https://ci.nodejs.org/job/node-test-pull-request/2958/ ... Green
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/301/ ... Lodash installation error appears unrelated

@addaleax

This comment has been minimized.

Member

addaleax commented Jun 8, 2016

LGTM

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 8, 2016

Thank you! will land tomorrow barring any other issues.

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 9, 2016

@bnoordhuis ... can I ask you to take a look at this quickly? Any nits with the node_constants.cc changes?

NODE_DEFINE_CONSTANT(target, INFLATERAW);
NODE_DEFINE_CONSTANT(target, UNZIP);
#define Z_MIN_WINDOWBITS 8

This comment has been minimized.

@bnoordhuis

bnoordhuis Jun 9, 2016

Member

Don't indent preprocessor directives.

#define Z_MIN_CHUNK 64
NODE_DEFINE_CONSTANT(target, Z_MIN_CHUNK);
#define Infinity std::numeric_limits<double>::infinity()

This comment has been minimized.

@bnoordhuis

bnoordhuis Jun 9, 2016

Member

Why the separate Infinity define?

This comment has been minimized.

@jasnell

jasnell Jun 9, 2016

Member

just for a bit of convenience. Definitely not critical

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 9, 2016

@bnoordhuis .. updated!

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Jun 11, 2016

LGTM

jasnell added a commit that referenced this pull request Jun 12, 2016

zlib: move constants into zlib.constants
zlib constants were previously being added to binding in node_zlib.cc.
This moves the zlib constants to node_constants.cc for consistency with
the recent constants refactoring:
  #6534

Adds require('zlib').constants to expose the constants
Docs-only deprecates the constants hung directly off require('zlib')
Removes a couple constants from the docs that apparently no longer
exist in the code

PR-URL: #7203
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell

This comment has been minimized.

Member

jasnell commented Jun 12, 2016

Landed in 197a465

@jasnell jasnell closed this Jun 12, 2016

@fhinkel fhinkel referenced this pull request Jul 2, 2016

Closed

doc: delete non-existing zlib constants #7520

3 of 3 tasks complete

fhinkel added a commit to fhinkel/node that referenced this pull request Jul 9, 2016

doc: delete non-existing zlib constants
Some constants in the zlib docs are not in the actual code:

zlib.Z_BINARY
zlib.Z_TEXT
zlib.Z_ASCII
zlib.Z_UNKNOWN

Also handled in nodejs#7203, but marked as
semver-major, so will not land in v6.x.

Fixes: nodejs#7204

Fishrock123 added a commit that referenced this pull request Jul 15, 2016

doc: delete non-existing zlib constants
Some constants in the zlib docs are not in the actual code:

zlib.Z_BINARY
zlib.Z_TEXT
zlib.Z_ASCII
zlib.Z_UNKNOWN

Also handled in #7203, but marked as
semver-major, so will not land in v6.x.

Fixes: #7204
PR-URL: #7520
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins added a commit that referenced this pull request Sep 9, 2016

doc: delete non-existing zlib constants
Some constants in the zlib docs are not in the actual code:

zlib.Z_BINARY
zlib.Z_TEXT
zlib.Z_ASCII
zlib.Z_UNKNOWN

Also handled in #7203, but marked as
semver-major, so will not land in v6.x.

Fixes: #7204
PR-URL: #7520
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins added a commit that referenced this pull request Sep 28, 2016

doc: delete non-existing zlib constants
Some constants in the zlib docs are not in the actual code:

zlib.Z_BINARY
zlib.Z_TEXT
zlib.Z_ASCII
zlib.Z_UNKNOWN

Also handled in #7203, but marked as
semver-major, so will not land in v6.x.

Fixes: #7204
PR-URL: #7520
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

rvagg added a commit that referenced this pull request Oct 18, 2016

doc: delete non-existing zlib constants
Some constants in the zlib docs are not in the actual code:

zlib.Z_BINARY
zlib.Z_TEXT
zlib.Z_ASCII
zlib.Z_UNKNOWN

Also handled in #7203, but marked as
semver-major, so will not land in v6.x.

Fixes: #7204
PR-URL: #7520
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

MylesBorins added a commit that referenced this pull request Oct 26, 2016

doc: delete non-existing zlib constants
Some constants in the zlib docs are not in the actual code:

zlib.Z_BINARY
zlib.Z_TEXT
zlib.Z_ASCII
zlib.Z_UNKNOWN

Also handled in #7203, but marked as
semver-major, so will not land in v6.x.

Fixes: #7204
PR-URL: #7520
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

@RafaelKr RafaelKr referenced this pull request Jan 11, 2017

Merged

[node] zlib: move constants into zlib.constants #13945

8 of 8 tasks complete

@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