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: correctly reference where to list hash algorithms. #9043

Closed
wants to merge 1 commit into from

Conversation

sstern6
Copy link
Contributor

@sstern6 sstern6 commented Oct 11, 2016

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().

@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 Oct 11, 2016
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Needs a few cleanups

Creates and returns a `Sign` object that uses the given `algorithm`. On
recent OpenSSL releases, `openssl list-public-key-algorithms` will
display the available signing algorithms. One example is `'RSA-SHA256'`.
Creates and returns a `Sign` object that uses the given `algorithm`. For available signing algorithms, please reference `crypto.getHashes()`.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: line wrap at 80 chars please :-)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will get on this, thanks for the feedback

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -1320,7 +1316,7 @@ console.log(alice_secret == bob_secret);
added: v0.9.3
-->

Returns an array with the names of the supported hash algorithms.
Returns an array with the names of the supported hash algorithms. For example, `RSA-SHA256`.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Creates and returns a `Sign` object that uses the given `algorithm`. On
recent OpenSSL releases, `openssl list-public-key-algorithms` will
display the available signing algorithms. One example is `'RSA-SHA256'`.
Creates and returns a `Sign` object that uses the given `algorithm`. For available signing algorithms, please reference `crypto.getHashes()`.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM with nits addressed.

@lpinca
Copy link
Member

lpinca commented Oct 12, 2016

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

@sstern6
Copy link
Contributor Author

sstern6 commented Oct 12, 2016

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

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

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1320,7 +1320,8 @@ console.log(alice_secret == bob_secret);
added: v0.9.3
-->

Returns an array with the names of the supported hash algorithms.
Returns an array with the names of the supported hash algorithms,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Trott updated

@sstern6
Copy link
Contributor Author

sstern6 commented Oct 14, 2016

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

Thanks

@jasnell jasnell self-assigned this Oct 17, 2016
jasnell pushed a commit that referenced this pull request Oct 17, 2016
PR-URL: #9043

Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 17, 2016

Landed in 3d294cf. Thank you!

@jasnell jasnell closed this Oct 17, 2016
jasnell pushed a commit that referenced this pull request Oct 17, 2016
PR-URL: #9043

Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
PR-URL: #9043

Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 11, 2016
PR-URL: #9043

Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
This was referenced Nov 22, 2016
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.

6 participants