crypto: synchronise with doc #14805

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

  // Explicit conversion for backward compatibility.
-  return this._handle.digest(`${outputEncoding}`);
+  return this._handle.digest(`${encoding}`);


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

+  return function(argOne, buffer) {
+    var key = argOne.key || argOne;
+    var passphrase = argOne.passphrase || null;
+    var padding = argOne.padding || defaultPadding;


options seemed like the better name here?

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

lib/crypto.js

-    var key = options.key || options;
-    var padding = options.padding || defaultPadding;
-    var passphrase = options.passphrase || null;
+function rsaBinder(method, defaultPadding) {


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

-exports.setEngine = function setEngine(id, flags) {
-  if (typeof id !== 'string')
+exports.setEngine = function setEngine(engine, flags) {
+  if (typeof engine !== 'string')
    throw new TypeError('"id" argument should be a string');


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

@@ -1102,8 +1106,11 @@ changes:
    description: Support for RSASSA-PSS and additional options was added.
 -->
 - `object` {string | Object}
+  - `key` {string | Buffer | TypedArray | DataView}


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

@@ -978,9 +980,7 @@ changes:
    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

 <!-- YAML
 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

 <!-- YAML
 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

@@ -378,7 +380,7 @@ The `decipher.setAuthTag()` method must be called before
 <!-- YAML
 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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: synchronise with doc #14805

crypto: synchronise with doc #14805

refack commented Aug 13, 2017 •

edited

Loading

refack commented Aug 13, 2017

refack commented Aug 13, 2017

refack commented Aug 13, 2017

mscdex Aug 13, 2017

refack Aug 13, 2017

mscdex commented Aug 13, 2017

addaleax Aug 13, 2017

addaleax Aug 13, 2017

addaleax Aug 13, 2017

refack Aug 13, 2017

refack commented Aug 13, 2017

addaleax commented Aug 13, 2017

refack commented Aug 13, 2017

thefourtheye Aug 14, 2017

BridgeAR commented Aug 31, 2017

refack Sep 13, 2017 •

edited

Loading

refack commented Sep 13, 2017

mscdex Sep 14, 2017

mscdex Sep 14, 2017

mscdex Sep 14, 2017 •

edited

Loading

mscdex commented Sep 14, 2017

addaleax left a comment

addaleax commented Sep 14, 2017

BridgeAR left a comment

BridgeAR commented Sep 19, 2017

BridgeAR commented Sep 28, 2017

BridgeAR commented Nov 22, 2017

crypto: synchronise with doc #14805

crypto: synchronise with doc #14805

Conversation

refack commented Aug 13, 2017 • edited Loading

Checklist

Affected core subsystem(s)

refack commented Aug 13, 2017

refack commented Aug 13, 2017

refack commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Aug 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Aug 13, 2017

addaleax commented Aug 13, 2017

refack commented Aug 13, 2017

Choose a reason for hiding this comment

BridgeAR commented Aug 31, 2017

refack Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

refack commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

mscdex commented Sep 14, 2017

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented Sep 14, 2017

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 19, 2017

BridgeAR commented Sep 28, 2017

BridgeAR commented Nov 22, 2017

refack commented Aug 13, 2017 •

edited

Loading

refack Sep 13, 2017 •

edited

Loading

mscdex Sep 14, 2017 •

edited

Loading