Skip to content

crypto: make _toBuf non-enumerable - #22551

Closed
tniessen wants to merge 1 commit into
nodejs:masterfrom
tniessen:crypto-make-tobuf-non-enumerable
Closed

crypto: make _toBuf non-enumerable#22551
tniessen wants to merge 1 commit into
nodejs:masterfrom
tniessen:crypto-make-tobuf-non-enumerable

Conversation

@tniessen

@tniessentniessen commented Aug 27, 2018

Copy link
Copy Markdown
Member

After landing #22501, I noticed that I did not make _toBuf non-enumerable. Following the discussion in #22519, I believe it should be, so here is that. Partially also for consistency with the rest of crypto.js.

cc @nodejs/crypto

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@tniessentniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 27, 2018
@nodejs-github-bot

nodejs-github-bot commented Aug 27, 2018

Copy link
Copy Markdown
Collaborator

@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/696/pipeline

@nodejs-github-botnodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Aug 27, 2018
@tniessen

tniessen commented Aug 27, 2018•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Full CI: https://ci.nodejs.org/job/node-test-pull-request/16787/
Resumed in: https://ci.nodejs.org/job/node-test-pull-request/16872/

@tniessentniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 27, 2018
addaleax
addaleax approved these changes Aug 27, 2018
View reviewed changes

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess this depends on a semver-major change, so it doesn’t really matter, but I don’t think we need to consider this one.

@tniessen

tniessen commented Aug 30, 2018

Copy link
Copy Markdown
MemberAuthor

@addaleax I thought we considered such changes as semver-major themselves, I am not sure though.

@tniessen

tniessen commented Aug 30, 2018

Copy link
Copy Markdown
MemberAuthor

Landed in 503fd55.

tniessen added a commit that referenced this pull request Aug 30, 2018
@tniessen
crypto: make _toBuf non-enumerable
503fd55
PR-URL: #22551
Refs: #22501
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@tniessentniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@cjihrigcjihrigcjihrig approved these changes
@trivikrtrivikrtrivikr approved these changes
+1 more reviewer
@thefourtheyethefourtheyethefourtheye approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cryptoIssues and PRs related to the crypto subsystem.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tniessen@nodejs-github-bot@jasnell@thefourtheye@addaleax@cjihrig@trivikr