Skip to content

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

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

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

Conversation

@ZaneHannanAU
Copy link
Contributor

@ZaneHannanAU ZaneHannanAU commented Jun 20, 2018

Checklist

Ensure consistency within documentation.

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations. labels Jun 20, 2018
@ZaneHannanAU ZaneHannanAU changed the title Ensure most Decipher methods return Decipher; not Cipher crypto: Ensure most Decipher methods return Decipher; not Cipher Jun 20, 2018
@ZaneHannanAU ZaneHannanAU changed the title crypto: Ensure most Decipher methods return Decipher; not Cipher doc/crypto: Ensure most Decipher methods return Decipher; not Cipher Jun 20, 2018
vsemozhetbyt
vsemozhetbyt reviewed Jun 20, 2018
View reviewed changes
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

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

Thank you.

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

@vsemozhetbyt vsemozhetbyt Jun 20, 2018 •
edited
Loading

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.

Copy link
Contributor Author

@ZaneHannanAU ZaneHannanAU Jun 20, 2018

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.

Copy link
Member

@BridgeAR BridgeAR Jun 21, 2018

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
Copy link
Contributor

vsemozhetbyt commented Jun 20, 2018

cc @nodejs/crypto

@BridgeAR BridgeAR requested a review from tniessen June 20, 2018 16:16
@Trott
Copy link
Member

Trott commented Jun 20, 2018

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.

@ZaneHannanAU ZaneHannanAU changed the title doc/crypto: Ensure most Decipher methods return Decipher; not Cipher doc/crypto: include options and correct return type for setAAD() Jun 20, 2018
@BridgeAR BridgeAR 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
Copy link
Contributor

vsemozhetbyt commented Jun 24, 2018

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

@ZaneHannanAU
Copy link
Contributor Author

ZaneHannanAU commented Jun 24, 2018

@vsemozhetbyt added for now sorry

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

@vsemozhetbyt vsemozhetbyt Jun 24, 2018

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
doc/api/crypto.md Outdated
- `buffer` {Buffer | TypedArray | DataView}
- `options` {Object}
- `options` {Object} [`stream.transform` options][]
- `plaintextLength`: {nunmber}
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jun 24, 2018

Choose a reason for hiding this comment

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

The same)

Copy link
Contributor Author

@ZaneHannanAU ZaneHannanAU Jun 24, 2018

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
Copy link
Contributor

vsemozhetbyt commented Jun 24, 2018

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>
@targos targos mentioned this pull request Jul 3, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@jasnell jasnell jasnell approved these changes

@tniessen tniessen tniessen approved these changes

+1 more reviewer

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

crypto Issues and PRs related to the crypto subsystem. doc Issues 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