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

Crypto docs don't explain the function signatures #12946

Closed
stuartpb opened this issue May 10, 2017 · 6 comments
Closed

Crypto docs don't explain the function signatures #12946

stuartpb opened this issue May 10, 2017 · 6 comments
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.

Comments

@stuartpb
Copy link

stuartpb commented May 10, 2017 •
edited
Loading

I just spent about an hour bashing my head against the wall because the documentation for crypto.publicEncrypt & co. don't explain that they return a new Buffer with the encrypted data instead of encrypting the passed-in Buffer in place.

Yes, in hindsight, this should have been obvious, but I don't get why these functions don't explain their return values when so much of the rest of core does.
@Trott Trott added doc Issues and PRs related to the documentations. crypto Issues and PRs related to the crypto subsystem. good first issue Issues that are suitable for first-time contributors. labels May 10, 2017
@Trott
Copy link
Member

Trott commented May 10, 2017 •
edited
Loading

Thanks for reporting this. If you or someone else wants to update the documentation and submit a pull request, the markdown file containing the relevant documentation is doc/api/crypto.md.

@fhalde fhalde mentioned this issue May 10, 2017
Closed
2 tasks
fhalde added a commit to fhalde/node that referenced this issue May 10, 2017
@fhalde
doc: Update doc of publicEncrypt method
06e7657 
As per nodejs#12946
the crypto doc for publicEncrypt doesn't tell
you whether the encryption happens in place or not.
@addaleax addaleax removed the good first issue Issues that are suitable for first-time contributors. label May 10, 2017
anchnk pushed a commit to anchnk/node that referenced this issue May 19, 2017
@fhalde
doc: update doc of publicEncrypt method
96b51d2 
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 <mscdex@mscdex.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@stuartpb
Copy link
Author

Okay, that fixes the documentation for publicEncrypt, but it doesn't touch the documentation for the other three functions like it (and I'd expect that there are more ambiguities like this in the docs right now).

Also, isn't there a formal way of documenting a function's return type?

@addaleax
Copy link
Member

Ah, right. Sorry, reopening.

Also, isn't there a formal way of documenting a function's return type?

I think there is, we have something like * Returns: {integer} The number of bytes contained within `string` for that.

@addaleax addaleax reopened this May 19, 2017
@refack refack added the good first issue Issues that are suitable for first-time contributors. label May 20, 2017
WilliamT1 added a commit to WilliamT1/node that referenced this issue May 25, 2017
@WilliamT1
Clarified documentation for crypto.publicDecrypt() and similar functi…
3585e6a 
…ons as per nodejs#12946
WilliamT1 added a commit to WilliamT1/node that referenced this issue May 25, 2017
@WilliamT1
doc: Clarified documentation for crypto.publicDecrypt() and similar f…
d680c52 
…unctions. PR URL: nodejs#12946
@WilliamT1 WilliamT1 mentioned this issue May 25, 2017
Closed
3 tasks
@jieyanhuang
Copy link
Contributor

Looks like the PR never got merged, does this still require attention? I can clean this up if help is still needed

@joyeecheung
Copy link
Member

@jieyanhuang Please do! Our contributing guide should provide enough guidance on how to sumbit a PR.

jieyanhuang added a commit to jieyanhuang/node that referenced this issue Oct 16, 2017
@jieyanhuang
doc: clarify documentation in crypto
29dbf82 
Clarify on the return values for crypto.publicEncrypt and similar
functions

Fixes: nodejs#12946
@jieyanhuang jieyanhuang mentioned this issue Oct 16, 2017
Closed
2 tasks
@jieyanhuang
Copy link
Contributor

@joyeecheung I've made a PR to address this. Let me know if I missed anything

@lance lance closed this as completed in a3a1068 Oct 16, 2017
targos pushed a commit that referenced this issue Oct 18, 2017
@jieyanhuang @targos
doc: add return values in crypto documentation
8a8e5c7 
Clarify return values for crypto.publicEncrypt and similar functions

PR-URL: #16229
Fixes: #12946
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 18, 2017
@jieyanhuang @addaleax
doc: add return values in crypto documentation
262fd32 
Clarify return values for crypto.publicEncrypt and similar functions

PR-URL: nodejs/node#16229
Fixes: nodejs/node#12946
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Milestone
No milestone
6 participants
@refack @stuartpb @Trott @addaleax @jieyanhuang @joyeecheung