Skip to content

doc/crypto: include options and correct return type for setAAD() - #21420

Closed
ZaneHannanAU wants to merge 4 commits into
nodejs:masterfrom
ZaneHannanAU:patch-2
Closed

doc/crypto: include options and correct return type for setAAD()#21420
ZaneHannanAU wants to merge 4 commits into
nodejs:masterfrom
ZaneHannanAU:patch-2

Conversation

@ZaneHannanAU

@ZaneHannanAUZaneHannanAU commented Jun 20, 2018

Copy link
Copy Markdown
Contributor
Checklist

Ensure consistency within documentation.

@nodejs-github-botnodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Jun 20, 2018
@ZaneHannanAUZaneHannanAU changed the title Ensure most Decipher methods return Decipher; not Ciphercrypto: Ensure most Decipher methods return Decipher; not CipherJun 20, 2018
@ZaneHannanAUZaneHannanAU changed the title crypto: Ensure most Decipher methods return Decipher; not Cipherdoc/crypto: Ensure most Decipher methods return Decipher; not CipherJun 20, 2018
vsemozhetbyt
vsemozhetbyt reviewed Jun 20, 2018
View reviewed changes

@vsemozhetbytvsemozhetbyt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you.

Comment threaddoc/api/crypto.md Outdated
-->
- `buffer` {Buffer | TypedArray | DataView}
- Returns: {Cipher} for method chaining.
- `options` {Object}

@vsemozhetbytvsemozhetbytJun 20, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems plaintextLength should be documented here as well, as a property of options, in a nested level.

@ZaneHannanAUZaneHannanAUJun 20, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah; I had originally planned on writing an AADOptions block but it was so short I didn't feel the need to.

Also I added CipherGCMOptions and CipherCCMOptions in @types/node; though not sure how I should go about that in @nodejs/node.

@BridgeARBridgeARJun 21, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you check other documentations, you'll see how options are handled there. Normally it just get's a short description what it stands for right after the type.

@vsemozhetbyt

vsemozhetbyt commented Jun 20, 2018

Copy link
Copy Markdown
Contributor

cc @nodejs/crypto

@BridgeAR
BridgeAR requested a review from tniessenJune 20, 2018 16:16
@Trott

Trott commented Jun 20, 2018

Copy link
Copy Markdown
Member

Hi @ZaneHannanAU! Welcome and thanks for the pull request!

Note for whoever lands this change: The commit message should probably not use ensure as that suggests a change in functionality whereas this is not changing behavior. It's correcting documentation and filling in an omission. So probably something like this:

doc: include options and correct return type for setAAD()

Something like that.

@ZaneHannanAUZaneHannanAU changed the title doc/crypto: Ensure most Decipher methods return Decipher; not Cipherdoc/crypto: include options and correct return type for setAAD()Jun 20, 2018
@BridgeARBridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jun 21, 2018
@vsemozhetbyt

vsemozhetbyt commented Jun 24, 2018

Copy link
Copy Markdown
Contributor

So should we land or should we wait for plaintextLength option to be documented in a formal way?

@ZaneHannanAU

ZaneHannanAU commented Jun 24, 2018

Copy link
Copy Markdown
ContributorAuthor

@vsemozhetbyt added for now sorry

vsemozhetbyt
vsemozhetbyt reviewed Jun 24, 2018
View reviewed changes
Comment threaddoc/api/crypto.md Outdated
- `buffer` {Buffer}
- `options` {Object}
- `options` {Object} [`stream.transform` options][]
- `plaintextLength`: {nunmber}

@vsemozhetbytvsemozhetbytJun 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: delete colon and {nunmber} -> {number} so we can run the last CI)

vsemozhetbyt
vsemozhetbyt reviewed Jun 24, 2018
View reviewed changes
Comment threaddoc/api/crypto.md Outdated
- `buffer` {Buffer | TypedArray | DataView}
- `options` {Object}
- `options` {Object} [`stream.transform` options][]
- `plaintextLength`: {nunmber}

@vsemozhetbytvsemozhetbytJun 24, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The same)

@ZaneHannanAUZaneHannanAUJun 24, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done :

vsemozhetbyt pushed a commit that referenced this pull request Jun 24, 2018
@ZaneHannanAU@vsemozhetbyt
doc: correct parameters, return types in crypto.md
c041fd2
PR-URL: #21420
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@vsemozhetbyt

vsemozhetbyt commented Jun 24, 2018

Copy link
Copy Markdown
Contributor

Landed in c041fd2
Thank you!

targos pushed a commit that referenced this pull request Jun 24, 2018
@ZaneHannanAU@targos
doc: correct parameters, return types in crypto.md
02bc99d
PR-URL: #21420
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targostargos mentioned this pull request Jul 3, 2018
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeARBridgeARBridgeAR left review comments
@jasnelljasnelljasnell approved these changes
@tniessentniessentniessen approved these changes
+1 more reviewer
@vsemozhetbytvsemozhetbytvsemozhetbyt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cryptoIssues and PRs related to the crypto subsystem.docIssues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ZaneHannanAU@vsemozhetbyt@Trott@jasnell@tniessen@BridgeAR@nodejs-github-bot