doc: correctly reference where to list hash algorithms. by sstern6 · Pull Request #9043 · nodejs/node · GitHub

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Docs.

Description of change

Fixes Issue: #9005.

Update crypto docs by removing inaccurate command explanation from crypto.createSign(algorithm) and crypto.createVerify(algorithm),
accurately referencing how to get available hash algorithms from crypto.getHashes().

Needs a few cleanups

Nit: line wrap at 80 chars please :-)

If we go with this approach, it would be best to make crypto.getHashes() a link to the right location.

Will get on this, thanks for the feedback

Using reference makes it sound like you should refer to the documentation for crypto.getHashes() but what is really meant is to run it to obtain the array of available algorithms, right? If so, instead of this:

For available signing algorithms, please reference crypto.getHashes().

...I'd prefer something like:

Use [crypto.getHashes()][] to obtain an array of names of the available signing algorithms.

@jasnell for linking to crypto.getHashes() would you reccomend putting a <a id="getHashes"></a> above crypto.getHashes() in the markdown, or do you have another preferred method?

Thanks

@sstern6 The linking is done via markdown. @jasnell is basically saying instead of using this markdown:

`crypto.getHashes()`

...use this markdown instead:

[`crypto.getHashes()`][]

Then make sure that the link is specified in the list of links at the bottom of the markdown document.

@sstern6 Some more info: That linking style is used throughout the doc and all (or nearly all) our other API docs. You can read about it at https://daringfireball.net/projects/markdown/syntax#link if you search for implicit link name shortcut.

thanks @Trott getting to that now. The link is already declared at the bottom of the page.

Updating the PR with all of the comments

The added text is a sentence fragment (which is fine for the first "sentence", but subsequent statements should be complete sentences). It seems unnecessary anyway. If we do want to include the example, I'd prefer:

Returns an array with the names of the supported hash algorithms, such as RSA-SHA256.

Using reference makes it sound like you should refer to the documentation for crypto.getHashes() but what is really meant is to run it to obtain the array of available algorithms, right? If so, instead of this:

For available signing algorithms, please reference crypto.getHashes().

...I'd prefer something like:

Use [crypto.getHashes()][] to obtain an array of names of the available signing algorithms.

LGTM with nits addressed.

@sstern6 can you please adjust the commit message to make it follow the guidelines? Thank you.

Thanks everyone, will have all of these addressed by end of day.

LGTM

Micro-nit: array with the names -> array of the names

@Trott updated

@Trott will have the micronit fixed by 4:30pst today.

Thanks

PR-URL: #9043 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Landed in 3d294cf. Thank you!

PR-URL: #9043 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Oct 11, 2016

jasnell requested changes Oct 11, 2016

View reviewed changes

Trott requested changes Oct 11, 2016

View reviewed changes

lpinca approved these changes Oct 12, 2016

View reviewed changes

sstern6 force-pushed the issue-9005 branch 2 times, most recently from 287e57d to 09162f8 Compare October 14, 2016 04:42

jasnell approved these changes Oct 14, 2016

View reviewed changes

Trott reviewed Oct 14, 2016

View reviewed changes

Trott approved these changes Oct 14, 2016

View reviewed changes

doc: update reference of where to list hash algorithms in crypto.md.

34d4bfc

sstern6 force-pushed the issue-9005 branch from 09162f8 to 34d4bfc Compare October 14, 2016 22:44

jasnell self-assigned this Oct 17, 2016

jasnell pushed a commit that referenced this pull request Oct 17, 2016

doc: update reference to list hash algorithms in crypto.md

3d294cf

PR-URL: #9043 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

jasnell closed this Oct 17, 2016

jasnell pushed a commit that referenced this pull request Oct 17, 2016

doc: update reference to list hash algorithms in crypto.md

0022bfe

PR-URL: #9043 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins pushed a commit that referenced this pull request Nov 11, 2016

doc: update reference to list hash algorithms in crypto.md

68ccc7a

PR-URL: #9043 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins pushed a commit that referenced this pull request Nov 11, 2016

doc: update reference to list hash algorithms in crypto.md

de2f050

PR-URL: #9043 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins added land-on-v4.x labels Nov 11, 2016

This was referenced Nov 22, 2016

v6.9.2 proposal #9735

Merged

v4.7.0 proposal #9736

Merged

doc: correctly reference where to list hash algorithms. by sstern6 · Pull Request #9043 · nodejs/node · GitHub

Uh oh!

Uh oh!

doc: correctly reference where to list hash algorithms. - #9043

doc: correctly reference where to list hash algorithms.#9043sstern6 wants to merge 1 commit intonodejs:masternodejs/node:masterfrom sstern6:issue-9005sstern6/node:issue-9005Copy head branch name to clipboard

Conversation

sstern6 commented Oct 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

lpinca commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sstern6 commented Oct 12, 2016

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstern6 commented Oct 14, 2016

Uh oh!

jasnell commented Oct 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

doc: correctly reference where to list hash algorithms.#9043
sstern6 wants to merge 1 commit into
nodejs:masterfrom
sstern6:issue-9005

sstern6 commented Oct 11, 2016 •
edited
Loading

lpinca commented Oct 12, 2016 •
edited
Loading