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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 4 commits into from
Closed

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

wants to merge 4 commits into from

Conversation

ZaneHannanAU
Copy link
Contributor

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
@@ -407,12 +407,17 @@ changes:
description: This method now returns a reference to `decipher`.
-->
- `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

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

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

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

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

@ZaneHannanAU
Copy link
Contributor Author

@vsemozhetbyt added for now sorry

vsemozhetbyt
vsemozhetbyt reviewed Jun 24, 2018
View reviewed changes
doc/api/crypto.md Outdated
@@ -245,7 +245,8 @@ once will result in an error being thrown.
added: v1.0.0
-->
- `buffer` {Buffer}
- `options` {Object}
- `options` {Object} [`stream.transform` options][]
- `plaintextLength`: {nunmber}
Copy link
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
doc/api/crypto.md Outdated
@@ -407,7 +408,8 @@ changes:
description: This method now returns a reference to `decipher`.
-->
- `buffer` {Buffer | TypedArray | DataView}
- `options` {Object}
- `options` {Object} [`stream.transform` options][]
- `plaintextLength`: {nunmber}
Copy link
Contributor

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

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

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

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@BridgeAR BridgeAR BridgeAR left review comments

@tniessen tniessen tniessen approved these changes

@jasnell jasnell jasnell approved these changes

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