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

crypto: crypto.create* - argument "options" is undocumented #14804

Closed
refack opened this issue Aug 13, 2017 · 5 comments
Closed

crypto: crypto.create* - argument "options" is undocumented #14804

refack opened this issue Aug 13, 2017 · 5 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. wip Issues and PRs that are still a work in progress.

Comments

@refack
Copy link
Contributor

refack commented Aug 13, 2017

  • Version: *
  • Platform: *
  • Subsystem: crypto

have a last optional argument options to control their stream behaviour

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

@refack refack added doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. labels Aug 13, 2017
@ashanhol
Copy link
Contributor

ashanhol commented Aug 15, 2017

To clarify @refack, these options are the OpenSSL options?
These are strings, correct?

Edit: If so, will submit a PR with these changes.

@refack
Copy link
Contributor Author

refack commented Aug 15, 2017

AFAICT from looking at the code, they are also stream options, as these methods wrap the underlining OpenSSL mechanism with a node transform stream.
For example:

node/lib/crypto.js

Lines 75 to 81 in 0d22858

exports.createHash = exports.Hash = Hash;
function Hash(algorithm, options) {
if (!(this instanceof Hash))
return new Hash(algorithm, options);
this._handle = new binding.Hash(algorithm);
LazyTransform.call(this, options);
}

where LazyTransform is
function LazyTransform(options) {
this._options = options;
this.writable = true;
this.readable = true;
}
util.inherits(LazyTransform, stream.Transform);

@refack
Copy link
Contributor Author

refack commented Aug 15, 2017

@ashanhol thanks for taking this on 🥇
There's an extra credit exercise to follow this up, but it requires some git spelunking, so it could wait for a future PR.

@refack refack added the wip Issues and PRs that are still a work in progress. label Aug 15, 2017
@ashanhol
Copy link
Contributor

Thanks @refack for your comments, they really helped. It's sometimes difficult to determine types.
I updated the branch and will submit a PR. I'm definitely down for the extra credit.

ashanhol added a commit to ashanhol/node that referenced this issue Aug 16, 2017
@ashanhol
Copy link
Contributor

@refack with the PR in works and waiting on the second round of approval, I'm available to jump into the next issue you had in mind.

MylesBorins pushed a commit that referenced this issue Sep 10, 2017
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>
MylesBorins pushed a commit that referenced this issue Sep 11, 2017
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>
MylesBorins pushed a commit that referenced this issue Sep 12, 2017
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>
addaleax pushed a commit to addaleax/node that referenced this issue Sep 13, 2017
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants