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

doc: add KeyObject to type #29689

Closed
wants to merge 1 commit into from
Closed

doc: add KeyObject to type #29689

wants to merge 1 commit into from

Conversation

exoego
Copy link
Contributor

@exoego exoego commented Sep 24, 2019

The key may optionally be a KeyObject of type secret

It is written in description, but not in type.

Checklist

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Sep 24, 2019
@Trott
Copy link
Member

Trott commented Sep 25, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3931/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 25, 2019
@Trott
Copy link
Member

Trott commented Sep 25, 2019

@tniessen @lpinca @devnexen @BridgeAR null has also been added to this PR now. A very quick re-review/re-approval would be great.

@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 25, 2019
exoego
exoego commented Sep 25, 2019
View reviewed changes
doc/api/crypto.md
@@ -1736,7 +1736,7 @@ changes:
-->

* `algorithm` {string}
* `key` {string | Buffer | TypedArray | DataView}
* `key` {string | Buffer | TypedArray | DataView | KeyObject}
* `iv` {string | Buffer | TypedArray | DataView | null}
Copy link
Contributor Author

@exoego exoego Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Note to reviewer:
| null was added in https://github.com/nodejs/node/pull/29684/files#diff-29db77451cd94ef9fbab42fd5b7499d5R1740

@tniessen
Copy link
Member

Could you rebase your branch to get rid of the merge commit? The changes themselves are fine, but PRs should usually not contain merge commits.

@Trott
Copy link
Member

Trott commented Sep 27, 2019

Could you rebase your branch to get rid of the merge commit? The changes themselves are fine, but PRs should usually not contain merge commits.

Rebased and force-pushed to get rid of the merge commit.

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3940/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 27, 2019
@Trott
Copy link
Member

Trott commented Sep 27, 2019

Landed in 5e1440c

@Trott Trott closed this Sep 27, 2019
Trott pushed a commit that referenced this pull request Sep 27, 2019
@Trott
doc: add KeyObject to type for crypto.createDecipheriv() argument
5e1440c 
PR-URL: #29689
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@exoego exoego deleted the patch-crypto branch September 28, 2019 02:58
targos pushed a commit that referenced this pull request Oct 1, 2019
@targos
doc: add KeyObject to type for crypto.createDecipheriv() argument
d86f10c 
PR-URL: #29689
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Oct 1, 2019
Merged
@beevelop beevelop mentioned this pull request Oct 1, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@devnexen devnexen devnexen approved these changes

@tniessen tniessen tniessen approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@exoego @Trott @tniessen @jasnell @lpinca @devnexen @BridgeAR @nodejs-github-bot