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

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

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

/cc @nodejs/documentation @nodejs/crypto

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

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

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.

options seemed like the better name here?

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.

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.

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.

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

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

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

Ping @refack

Description of Object case in second paragraph L988

@mscdex @addaleax @thefourtheye addressed comments PTAL

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

Ditto

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

My comment about changing the code still stands.

@refack Thank you!

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

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

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

Ping @refack

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

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

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

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

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

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

Uh oh!

Uh oh!

crypto: synchronise with doc - #14805

crypto: synchronise with doc#14805refack wants to merge 4 commits intonodejs:masternodejs/node:masterfrom refack:crypo-doc-bufrefack/node:crypo-doc-bufCopy head branch name to clipboard

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

crypto: synchronise with doc#14805
refack wants to merge 4 commits into
nodejs:masterfrom
refack:crypo-doc-buf

refack commented Aug 13, 2017 •
edited
Loading

refack Sep 13, 2017 •
edited
Loading

mscdex Sep 14, 2017 •
edited
Loading