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: clarified documentation for crypto.publicEncrypt() and similar functions #13212

Closed
wants to merge 2 commits into from

Conversation

WilliamT1
Copy link

@WilliamT1 WilliamT1 commented May 25, 2017

Fixes: #12946

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@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 25, 2017
@WilliamT1 WilliamT1 changed the title Clarified documentation for crypto.publicEncrypt() and similar functions as per nodejs#12946 doc: clarified documentation for crypto.publicEncrypt() and similar functions as per nodejs#12946 May 25, 2017
@mscdex
Copy link
Contributor

mscdex commented May 25, 2017

I think if we're going to convert at least one document, it should be done for all functions and not just a select few. Also, the format for documenting the return value isn't quite in line with that of other built-in module docs (for example some of the methods in the buffer documentation).

@refack refack changed the title doc: clarified documentation for crypto.publicEncrypt() and similar functions as per nodejs#12946 doc: clarified documentation for crypto.publicEncrypt() and similar functions May 25, 2017
@refack
Copy link
Contributor

refack commented May 25, 2017

@WilliamT1 Thank you for your contribution 🥇 and welcome 🎉
I've made some edits to your title and first comment (moved the reference to the 12946 issue) so it'll use GitHub cross reference feature (since issue numbers are ignored in titles).

As you see, once you open a PR the other contributors will give you comments. That our way to make sure that your contribution brings the maximal benefit to the codebase. It's all covered in our CONTRIBUTING.md guide.

For this PR you should adjust the commit message (as per step 3 in the guide), then you can check the box in the first comment.
Also you should consider following @mscdex's comment and, first adjest the format of the changes you made to fit the other examples in the file (like for example the signature of certificate.exportChallenge), and then maybe have a go at documenting the rest of the functions 👍

Good luck and thank you again.

@refack refack self-assigned this Jun 2, 2017
@refack
Copy link
Contributor

refack commented Jun 2, 2017

@refack
Copy link
Contributor

refack commented Jun 2, 2017

@WilliamT1 are you interested in following up on the reviews?

@BridgeAR
Copy link
Member

There was no update on this for a long time. Closing therefore.

@WilliamT1 thanks a lot for your contribution anyway and if you would like to follow up on this, please feel free to reopen!

@BridgeAR BridgeAR closed this Aug 27, 2017
@refack refack removed their assignment Oct 20, 2018
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.

Crypto docs don't explain the function signatures
6 participants