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

doc: correct crypto.randomFill() and randomFillSync() #21550

Closed
wants to merge 3 commits into from
Closed

doc: correct crypto.randomFill() and randomFillSync() #21550

wants to merge 3 commits into from

Conversation

Flarna
Copy link
Member

@Flarna Flarna commented Jun 26, 2018

Correct return type of crypto.randomFillSync() which is of same type as passed as buffer argument.

Correct samples for randomFill() and randomFillSync() using a TypeArray or DataView as these types don't support .toString(encoding).

@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 26, 2018
@@ -2061,13 +2061,13 @@ Any `TypedArray` or `DataView` instance may be passed as `buffer`.

```js
const a = new Uint32Array(10);
console.log(crypto.randomFillSync(a).toString('hex'));
console.log(Buffer.from(crypto.randomFillSync(a).buffer).toString('hex'));
Copy link
Member

Choose a reason for hiding this comment

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

Just using .buffer works in these cases, but I’m not sure we should include it in our documentation like this – most of the time when you’re doing this, you’ll want to consider .byteLength and .byteOffset as well?

Copy link
Member Author

@Flarna Flarna Jun 27, 2018

Choose a reason for hiding this comment

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

I just modified the samples to work without thinking to much about it. Printing a Float64Array formatted as 'hex' is usually not a typical use. I will update the samples to use also .byteLength and .byteOffset but I'm not sure if doc of randomFillSync() is the right place to show how to work with typed arrays.

Correct return type of `crypto.randomFillSync()` which is of same type as
passed as `buffer` argument.

Correct samples for `randomFill()` and `randomFillSync()` using a `TypeArray`
or `DataView` as these types don't support `.toString(encoding)`.
@addaleax
Copy link
Member

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 18, 2018
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM even though I wonder if we should explain the example at least once?

@trivikr
Copy link
Member

trivikr commented Aug 5, 2018

Landed in acc633c

@trivikr trivikr closed this Aug 5, 2018
trivikr pushed a commit that referenced this pull request Aug 5, 2018
Correct return type of `crypto.randomFillSync()` which is of same type as
passed as `buffer` argument.

Correct samples for `randomFill()` and `randomFillSync()` using a `TypeArray`
or `DataView` as these types don't support `.toString(encoding)`.

PR-URL: #21550
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Flarna Flarna deleted the doc_crypto_randomfill branch August 5, 2018 19:47
targos pushed a commit that referenced this pull request Aug 6, 2018
Correct return type of `crypto.randomFillSync()` which is of same type as
passed as `buffer` argument.

Correct samples for `randomFill()` and `randomFillSync()` using a `TypeArray`
or `DataView` as these types don't support `.toString(encoding)`.

PR-URL: #21550
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants