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: clarify allowed encoding parameter types #24230

Closed
wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

The code calls them encoding (mostly), and they are the encoding types
supported by Buffer, so call them that. Also fix the incorrect
enumerations of their possible values, which weren't up to date with the
values actually supported.

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

@nodejs-github-bot nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Nov 7, 2018
@sam-github sam-github added the doc Issues and PRs related to the documentations. label Nov 7, 2018
@mscdex
Copy link
Contributor

mscdex commented Nov 7, 2018

I think keeping the 'output' prefix is helpful in making it more clear that it's about what the function produces and not something to do with the contents of privateKey.

@sam-github
Copy link
Contributor Author

@mscdex so you suggest s/outputFormat/outputEncoding/?

@mscdex
Copy link
Contributor

mscdex commented Nov 8, 2018

@sam-github I'm fine with either one

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM % naming nit & anchor fixing

@sam-github sam-github force-pushed the fix-sign-encoding-docs branch 2 times, most recently from cbe9b37 to 7380867 Compare November 9, 2018 17:56
@sam-github sam-github changed the title doc: use real name for sign/verify encoding args doc: clarify allowed encoding parameter types Nov 9, 2018
@sam-github
Copy link
Contributor Author

@vsemozhetbyt How did you find those link errors? The markdown linter isn't find them :-(.

PTAL, I think I addressed all comments.

@vsemozhetbyt
Copy link
Contributor

I usually just grep old anchors in the repository for such signature changes)

This fixes the incorrect enumerations of their possible values, which
weren't up to date with the values actually supported. Also renamed
two arguments that used "format" when they meant "encoding".
@sam-github sam-github removed the request for review from addaleax November 9, 2018 18:36
@sam-github
Copy link
Contributor Author

@vsemozhetbyt is a travis CI build sufficient for doc PRs, or do I need a full jenkins CI?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Nov 9, 2018

Sorry, I just know that CI-lite is sufficient, i.e. linux-one + linter. I am not very aware of our different CI flavors.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Format LGTM)

@vsemozhetbyt
Copy link
Contributor

@sam-github
Copy link
Contributor Author

ci-lite is what I was looking for, thanks.

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

Trott commented Nov 9, 2018

Landed in 426ca08

@Trott Trott closed this Nov 9, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 9, 2018
This fixes the incorrect enumerations of their possible values, which
weren't up to date with the values actually supported. Also renamed
two arguments that used "format" when they meant "encoding".

PR-URL: nodejs#24230
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@sam-github sam-github deleted the fix-sign-encoding-docs branch November 10, 2018 00:13
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
This fixes the incorrect enumerations of their possible values, which
weren't up to date with the values actually supported. Also renamed
two arguments that used "format" when they meant "encoding".

PR-URL: #24230
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
This fixes the incorrect enumerations of their possible values, which
weren't up to date with the values actually supported. Also renamed
two arguments that used "format" when they meant "encoding".

PR-URL: nodejs#24230
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
codebytere pushed a commit that referenced this pull request Jan 12, 2019
This fixes the incorrect enumerations of their possible values, which
weren't up to date with the values actually supported. Also renamed
two arguments that used "format" when they meant "encoding".

PR-URL: #24230
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
@codebytere codebytere mentioned this pull request Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants