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.

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

doc: Update doc of publicEncrypt method #12947

Closed
wants to merge 1 commit into from
Closed

Conversation

fhalde
Copy link
Contributor

@fhalde fhalde commented May 10, 2017

As per #12946
the crypto doc for publicEncrypt doesn't tell
you whether the encryption happens in place or not.

Checklist
Affected core subsystem(s)

doc

@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 May 10, 2017
Copy link
Contributor

@danbev danbev left a comment

Choose a reason for hiding this comment

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

Nit: could you limit the line length to 80 characters so that is consistent?

@fhalde
Copy link
Contributor Author

fhalde commented May 10, 2017

Yupp

@@ -1665,7 +1665,8 @@ added: v0.11.14
`RSA_PKCS1_PADDING`, or `crypto.constants.RSA_PKCS1_OAEP_PADDING`.
- `buffer` {Buffer | TypedArray | DataView}

Encrypts `buffer` with `public_key`.
Encrypts the content of `buffer` with `public_key` and returns a new
buffer with encrypted content.
Copy link
Contributor

@mscdex mscdex May 10, 2017

Choose a reason for hiding this comment

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

s/buffer/[`Buffer`][]/

@mscdex
Copy link
Contributor

mscdex commented May 10, 2017

While you're in there, would you mind changing private_key to public_key in both of the public*() method parameter descriptions?

As per nodejs#12946
the crypto doc for publicEncrypt doesn't tell
you whether the encryption happens in place or not.
@fhalde
Copy link
Contributor Author

fhalde commented May 10, 2017

done & done @mscdex

@mscdex
Copy link
Contributor

mscdex commented May 10, 2017

LGTM

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member

Landed in eff9252

@addaleax addaleax closed this May 19, 2017
addaleax pushed a commit that referenced this pull request May 19, 2017
As per #12946
the crypto doc for publicEncrypt doesn't tell
you whether the encryption happens in place or not.

Fixes: #12946
PR-URL: #12947
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
As per nodejs#12946
the crypto doc for publicEncrypt doesn't tell
you whether the encryption happens in place or not.

Fixes: nodejs#12946
PR-URL: nodejs#12947
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@jasnell jasnell mentioned this pull request May 28, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins
Copy link
Contributor

v6.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants