Crypto documentation update #14846

ashanhol · 2017-08-16T00:32:14Z

Updated crypto docs to include options argument.
Fixes: #14804

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

Fixes: nodejs#14804

refack · 2017-08-16T00:39:37Z

@ashanhol welcome, and thank you for your contribution 🥇

refack · 2017-08-16T00:40:02Z

/cc @nodejs/documentation @nodejs/crypto @nodejs/streams

vsemozhetbyt · 2017-08-16T00:59:22Z

Hi, @ashanhol. Thank you for improving the docs.
A nit: if I recall correctly, in type labels we use lower case for primitives only, so {object} should possibly be {Object}.

mscdex · 2017-08-16T01:04:46Z

doc/api/crypto.md

 <!-- YAML
 added: v0.1.94
 -->
 - `algorithm` {string}
 - `password` {string | Buffer | TypedArray | DataView}
+- `options` {object} combined [OpenSSL options][] and [`stream.transform` options][]


I think it should be stream.Transform since we're referencing a constructor.

TimothyGu

LGTM assuming @vsemozhetbyt's nit and this one is addressed.

TimothyGu · 2017-08-16T01:04:51Z

doc/api/crypto.md

@@ -2247,3 +2258,5 @@ the `crypto`, `tls`, and `https` modules and are generally specific to OpenSSL.
 [initialization vector]: https://en.wikipedia.org/wiki/Initialization_vector
 [stream-writable-write]: stream.html#stream_writable_write_chunk_encoding_callback
 [stream]: stream.html
+[OpenSSL options]: #openssl-options
+[`stream.transform` options]: stream.html#stream_new_stream_transform_options


Can you sort the references ASCIIbetically? See the references that are already there.

TimothyGu

Sorry, but I'm going to have to renege on my LGTM earlier. Noticed some more issues with this...

TimothyGu · 2017-08-16T01:10:50Z

doc/api/crypto.md

 <!-- YAML
 added: v0.1.92
 -->
 - `algorithm` {string}
+- `options` {object} combined [OpenSSL options][] and [`stream.transform` options][]


Actually for Sign it's stream.Writable:

node/lib/crypto.js

Lines 285 to 292 in 0d22858

function Sign(algorithm, options) {

if (!(this instanceof Sign))

return new Sign(algorithm, options);

this._handle = new binding.Sign();

this._handle.init(algorithm);

stream.Writable.call(this, options);

}

TimothyGu · 2017-08-16T01:11:18Z

doc/api/crypto.md

 <!-- YAML
 added: v0.1.92
 -->
 - `algorithm` {string}
+- `options` {object} combined [OpenSSL options][] and [`stream.transform` options][]


It's stream.Writable for Verify too.

TimothyGu · 2017-08-16T01:14:42Z

doc/api/crypto.md

 <!-- YAML
 added: v0.1.94
 -->
 - `algorithm` {string}
 - `password` {string | Buffer | TypedArray | DataView}
+- `options` {object} combined [OpenSSL options][] and [`stream.transform` options][]


The options object isn't actually combined, but only used for streams:

node/lib/crypto.js

Lines 136 to 145 in 0d22858

function Cipher(cipher, password, options) {

if (!(this instanceof Cipher))

return new Cipher(cipher, password, options);

this._handle = new binding.CipherBase(true);

this._handle.init(cipher, toBuf(password));

this._decoder = null;

LazyTransform.call(this, options);

}

The first instance of options is used when the function is not called with new (it's passed back into itself).

Same goes for the other classes.

vsemozhetbyt · 2017-08-16T01:16:06Z

It seems, there is one missing step. If we change headings in our docs, this also automatically changes anchors for href hashes. So for each changed heading we should search all the docs for previous hashes and update them.

You can get the old hashes from the TOC in the https://nodejs.org/api/crypto.html

For example the old one for ### crypto.createCipher(algorithm, password) was #crypto_crypto_createcipher_algorithm_password. So the new one for ### crypto.createCipher(algorithm, password[, options]) would be the #crypto_crypto_createcipher_algorithm_password_options if I get this right. There is a reference with the old hash in the crypto.md, but we also possibly should check all the doc files for such cases and update all the cross-links. I think, grepping all the *.md files with some tool for this fragments will do.

refack · 2017-08-16T01:25:01Z

@ashanhol what I forgot to mention before is that documentation changes, while easy to do are more difficult to validate. With code we can write tests and the CI can validate the change. With documentation the only way to validate is with human eyes, so don't be dismayed.

refack · 2017-08-16T01:35:57Z

It seems, there is one missing step. If we change headings in our docs, this also changes anchors and href hashes. So for each changed heading we should search all the docs for previous hashes and update them.

That's a real pain, we need to figure out an automatic way to do this...

vsemozhetbyt · 2017-08-16T01:40:25Z

@refack If the #12756 makes it, we can at least think about a linting rule for such outdated hashes inside the doc system.

mcollina

LGTM with all the nits addressed.

ashanhol · 2017-08-16T17:29:45Z

Thank you all for the feedback, I've made the suggested changes and grep'd the docs to make sure the old header links are updated.
@vsemozhetbyt @TimothyGu

jasnell · 2017-08-23T18:23:11Z

Ping @TimothyGu ... have your concerns been addressed?

BridgeAR · 2017-09-01T00:18:35Z

Ping @TimothyGu

BridgeAR · 2017-09-03T22:19:16Z

I take that as a yes.

@ashanhol please rebase this as we otherwise can not land this cleanly. Currently there is a merge commit in here.

ashanhol · 2017-09-05T20:28:14Z

Hi @BridgeAR, seems a couple commits got duplicated somewhere. I did an interactive rebase and removed them so there's no merges. 👍

PR-URL: #14846 Fixes: #14804 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

tniessen · 2017-09-07T11:19:40Z

Landed in 4477155, thank you for your first contribution @ashanhol! 🎉

Some tips for future contributions:

If you can, it would be great if you format the commit title and message of the first commit of a PR according to our contribution guidelines.
You can use git's whitespace options to automatically check for trailing whitespace, and it is usually a good idea to configure your editor to automatically remove or highlight unnecessary whitespace. Usually, such nits are not a problem, but they can make merging your changes a little more difficult.

PR-URL: #14846 Fixes: #14804 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

PR-URL: nodejs#14846 Fixes: nodejs#14804 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>

ashanhol added 2 commits August 15, 2017 20:24

Updated crypto docs to include options argument

526c6f6

Fixes: nodejs#14804

Fixed formatting errors and corrected types

a2806b4

nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Aug 16, 2017

refack approved these changes Aug 16, 2017

View reviewed changes

refack self-assigned this Aug 16, 2017

vsemozhetbyt approved these changes Aug 16, 2017

View reviewed changes

mscdex reviewed Aug 16, 2017

View reviewed changes

TimothyGu reviewed Aug 16, 2017

View reviewed changes

jasnell approved these changes Aug 16, 2017

View reviewed changes

mcollina approved these changes Aug 16, 2017

View reviewed changes

thefourtheye approved these changes Aug 16, 2017

View reviewed changes

lpinca approved these changes Aug 26, 2017

View reviewed changes

BridgeAR approved these changes Sep 3, 2017

View reviewed changes

ashanhol added 2 commits September 5, 2017 16:07

Made changes as requested in PR

3843866

removed uneccessary link

9a2987e

ashanhol force-pushed the cryptoDocUpdate branch from 2fe8d86 to 9a2987e Compare September 5, 2017 20:21

benjamingr approved these changes Sep 6, 2017

View reviewed changes

jasnell approved these changes Sep 6, 2017

View reviewed changes

tniessen closed this Sep 7, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins added the dont-land-on-v6.x label Sep 20, 2017

refack removed their assignment Oct 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crypto documentation update #14846

Crypto documentation update #14846

ashanhol commented Aug 16, 2017

refack commented Aug 16, 2017

refack commented Aug 16, 2017

vsemozhetbyt commented Aug 16, 2017

mscdex Aug 16, 2017

TimothyGu left a comment

TimothyGu Aug 16, 2017

TimothyGu left a comment

TimothyGu Aug 16, 2017

TimothyGu Aug 16, 2017

TimothyGu Aug 16, 2017

vsemozhetbyt commented Aug 16, 2017 •

edited

Loading

refack commented Aug 16, 2017

refack commented Aug 16, 2017

vsemozhetbyt commented Aug 16, 2017

mcollina left a comment

ashanhol commented Aug 16, 2017

jasnell commented Aug 23, 2017

BridgeAR commented Sep 1, 2017

BridgeAR commented Sep 3, 2017

ashanhol commented Sep 5, 2017

tniessen commented Sep 7, 2017

	function Sign(algorithm, options) {
	if (!(this instanceof Sign))
	return new Sign(algorithm, options);
	this._handle = new binding.Sign();
	this._handle.init(algorithm);

	stream.Writable.call(this, options);
	}

	function Cipher(cipher, password, options) {
	if (!(this instanceof Cipher))
	return new Cipher(cipher, password, options);
	this._handle = new binding.CipherBase(true);

	this._handle.init(cipher, toBuf(password));
	this._decoder = null;

	LazyTransform.call(this, options);
	}

Crypto documentation update #14846

Crypto documentation update #14846

Conversation

ashanhol commented Aug 16, 2017

Checklist

Affected core subsystem(s)

refack commented Aug 16, 2017

refack commented Aug 16, 2017

vsemozhetbyt commented Aug 16, 2017

mscdex Aug 16, 2017

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Aug 16, 2017

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Aug 16, 2017

Choose a reason for hiding this comment

TimothyGu Aug 16, 2017

Choose a reason for hiding this comment

TimothyGu Aug 16, 2017

Choose a reason for hiding this comment

vsemozhetbyt commented Aug 16, 2017 • edited Loading

refack commented Aug 16, 2017

refack commented Aug 16, 2017

vsemozhetbyt commented Aug 16, 2017

mcollina left a comment

Choose a reason for hiding this comment

ashanhol commented Aug 16, 2017

jasnell commented Aug 23, 2017

BridgeAR commented Sep 1, 2017

BridgeAR commented Sep 3, 2017

ashanhol commented Sep 5, 2017

tniessen commented Sep 7, 2017

vsemozhetbyt commented Aug 16, 2017 •

edited

Loading