crypto: synchronise with doc by refack · Pull Request #14805 · nodejs/node

refack · 2017-08-13T12:25:18Z

Fixes: #14800

Improve description of formal args in doc
Rename formal arguments to match doc
Merge rsa binding factories

Checklist

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

Affected core subsystem(s)

crypto,doc

refack · 2017-08-13T12:28:40Z

Quick sanity: https://ci.nodejs.org/job/node-test-commit-linuxone/7933/

refack · 2017-08-13T12:29:01Z

/cc @nodejs/documentation @nodejs/crypto

refack · 2017-08-13T13:01:45Z

Quick recheck: https://ci.nodejs.org/job/node-test-commit-linuxone/7934/

mscdex · 2017-08-13T13:12:15Z

lib/crypto.js

Why was this changed to a more generic name?

To sync with the docs - https://nodejs.org/api/crypto.html#crypto_hash_digest_encoding

node/doc/api/crypto.md

Line 783 in 2e7ccc2

### hash.digest([encoding])

Changing the docs causes the hrefs to change

mscdex · 2017-08-13T13:17:42Z

I'm not sure most of these changes are worth it. It's not necessary to ensure the documentation parameter names exactly match what are used internally.

addaleax · 2017-08-13T13:15:13Z

lib/crypto.js

options seemed like the better name here?

addaleax · 2017-08-13T13:15:23Z

lib/crypto.js

Um … “binder”?

Elsewhere in the code we name these things makeFoo, so you could call it makeRSAMethod or so if you at set on wanting to change the names.

addaleax · 2017-08-13T13:17:53Z

lib/crypto.js

This TypeError gets really weird now.

Also, if anything, engineID is probably better.

I know, there is at least another place like this.
I'll do the migration to internal/errors and fix these.

refack · 2017-08-13T13:25:19Z

I'm not sure most of these changes are worth it. It's not necessary to ensure the documentation parameter names exactly match what are used internally.

The reasoning to synchronize is to help with debugging, where you are exposed the "internal" arguments' names.
In order not to break the docs linking I synchronized in one direction doc -> js.

addaleax · 2017-08-13T13:28:14Z

@refack It’s okay to break links in the documentation so long as it’s updated throughout our docs.

refack · 2017-08-13T13:31:25Z

@refack It’s okay to break links in the documentation so long as it’s updated throughout our docs.

It's more delicate work. I'll give it another look if there are places where it's obviously better to break the docs. Also it breaks "permalinks" that might be embedded in other sites, so the change should really be worth it and I'd even categorize it semver-major.

thefourtheye · 2017-08-14T03:49:56Z

doc/api/crypto.md

This change has to be done in the list below the paragraph. All the options are explained there.

BridgeAR · 2017-08-31T16:54:54Z

Ping @refack

refack · 2017-09-13T23:03:57Z

doc/api/crypto.md

    description: Support for RSASSA-PSS and additional options was added.
 -->
 - `privateKey` {string | Object}
-  - `key` {string}


Description of Object case in second paragraph L988

refack · 2017-09-13T23:05:25Z

@mscdex @addaleax @thefourtheye addressed comments PTAL

mscdex · 2017-09-14T00:27:58Z

doc/api/crypto.md

 added: v0.11.8
 -->
 - `spkac` {string | Buffer | TypedArray | DataView}
+- `encoding` {string} (Default `utf-8`) used to encode the `spkac` into a Buffer


For string literal values, single quotes should be added inside the backticks: `'utf-8'`

mscdex · 2017-09-14T00:28:14Z

doc/api/crypto.md

 added: v0.11.8
 -->
 - `spkac` {string | Buffer | TypedArray | DataView}
+- `encoding` {string} (Default `utf-8`) used to encode the `spkac` into a Buffer


mscdex · 2017-09-14T00:31:28Z

doc/api/crypto.md

 added: v0.7.1
 -->
- `autoPadding` {boolean} Defaults to `true`.
+- `autoPadding` {boolean} (Default `true`).


If we're going to switch these, we should do so consistently. There are other instances in this document.

mscdex · 2017-09-14T00:33:27Z

My comment about changing the code still stands.

addaleax

@refack Thank you!

addaleax · 2017-09-14T10:29:38Z

@refack Re: the code changes, it might be better to contribute that into #15231 than to do it in this PR

BridgeAR

The doc changes LGTM but I would also prefer not to include the changes in crypto (besides the consolidation of the rsaPublic and rsaPrivate functions).

BridgeAR · 2017-09-19T21:22:13Z

This needs a rebase. I think the doc changes could land soon otherwise.

BridgeAR · 2017-09-28T06:08:23Z

Ping @refack

BridgeAR · 2017-11-22T14:02:35Z

Closing due to long inactivity. @refack please feel free to reopen if you want to further pursue this!

nodejs-github-bot added the crypto Issues and PRs related to the crypto subsystem. label Aug 13, 2017

refack mentioned this pull request Aug 13, 2017

password and salt parameter allows both string and buffer, but the doc only says string #14800

Closed

refack force-pushed the crypo-doc-buf branch from 4948fcf to 0306663 Compare August 13, 2017 13:00

mscdex reviewed Aug 13, 2017

View reviewed changes

addaleax reviewed Aug 13, 2017

View reviewed changes

refack added the doc Issues and PRs related to the documentations. label Aug 13, 2017

thefourtheye reviewed Aug 14, 2017

View reviewed changes

doc/api/crypto.md Outdated

Copy link

Contributor

thefourtheye Aug 14, 2017

Choose a reason for hiding this comment

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

This change has to be done in the list below the paragraph. All the options are explained there.

refack added 3 commits September 13, 2017 15:33

crypto: synchronize with doc

6c02d3b

[sqwsh] better names (addaleax)

c54ea14

[sqwsh] tiny simplification

88716d9

refack force-pushed the crypo-doc-buf branch from f676054 to 8d8f61c Compare September 13, 2017 22:58

[squash] comments

4f24fe1

refack force-pushed the crypo-doc-buf branch from 8d8f61c to 4f24fe1 Compare September 13, 2017 23:02

refack commented Sep 13, 2017

View reviewed changes

mscdex reviewed Sep 14, 2017

View reviewed changes

addaleax approved these changes Sep 14, 2017

View reviewed changes

BridgeAR approved these changes Sep 19, 2017

View reviewed changes

refack self-assigned this Sep 23, 2017

BridgeAR closed this Nov 22, 2017

refack deleted the crypo-doc-buf branch September 5, 2018 22:30

refack removed their assignment Oct 12, 2018

Uh oh!

Conversation

refack commented Aug 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Aug 13, 2017

Uh oh!

refack commented Aug 13, 2017

Uh oh!

refack commented Aug 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Aug 13, 2017

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!

refack commented Aug 13, 2017

Uh oh!

addaleax commented Aug 13, 2017

Uh oh!

refack commented Aug 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Aug 31, 2017

Uh oh!

refack Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Sep 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Sep 14, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 14, 2017

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 19, 2017

Uh oh!

BridgeAR commented Sep 28, 2017

Uh oh!

BridgeAR commented Nov 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Comments

refack commented Aug 13, 2017 •

edited

Loading

refack Sep 13, 2017 •

edited

Loading

mscdex Sep 14, 2017 •

edited

Loading