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: format ArrayBufferView as inline code #17595

Closed
wants to merge 2 commits into from
Closed

doc: format ArrayBufferView as inline code #17595

wants to merge 2 commits into from

Conversation

tniessen
Copy link
Member

ArrayBufferView should be formatted as ArrayBufferView.

Checklist
Affected core subsystem(s)

doc

@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 Dec 10, 2017
@tniessen tniessen added the fast-track PRs that do not need to wait for 48 hours to land. label Dec 10, 2017
TimothyGu
TimothyGu approved these changes Dec 10, 2017
View reviewed changes
doc/api/crypto.md Outdated
@@ -1847,7 +1847,7 @@ added: v7.10.0
changes:
- version: v9.0.0
pr-url: https://github.com/nodejs/node/pull/15231
description: The `buffer` argument may be any ArrayBufferView
description: The `buffer` argument may be any `ArrayBufferView`
Copy link
Member

Choose a reason for hiding this comment

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

I know I probably added this myself, but I now actually prefer ArrayBufferView to not be mentioned at all in the documentation, as it's not a JavaScript type (only a Web IDL one). I'd say something like "may be any TypedArray or DataView".

doc/api/crypto.md Outdated
@@ -1847,7 +1847,7 @@ added: v7.10.0
changes:
- version: v9.0.0
pr-url: https://github.com/nodejs/node/pull/15231
description: The `buffer` argument may be any ArrayBufferView
description: The `buffer` argument may be any `ArrayBufferView`
-->

* `buffer` {Buffer|Uint8Array|ArrayBufferView} Must be supplied.
Copy link
Member

Choose a reason for hiding this comment

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

While at it, Uint8Array is now redundant. Ditto below.

Copy link
Member

Choose a reason for hiding this comment

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

So is Buffer. ;) The thing is, at this point most Node developers don’t know that Buffer is a kind of Uint8Array, so it makes sense to list them both. In the same vein, keeping ArrayBufferView listed explicitly might be redundant but helpful, since 99 % of the time the user is going to pass in an Uint8Array.

Copy link
Member

Choose a reason for hiding this comment

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

But if it is fairly obvious that Uint8Array is a type of TypedArray. In conjunction with #17595 (comment), my preference would be {Buffer|TypedArray|DataView}.

@tniessen
Copy link
Member Author

@TimothyGu @addaleax PTAL

TimothyGu
TimothyGu approved these changes Dec 11, 2017
View reviewed changes
Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

LGTM.

@tniessen
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/12055/

@tniessen
Copy link
Member Author

Landed in def6072.

@tniessen tniessen closed this Dec 12, 2017
tniessen added a commit that referenced this pull request Dec 12, 2017
@tniessen
doc: replace ArrayBufferView in crypto
def6072 
PR-URL: #17595
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@tniessen @MylesBorins
doc: replace ArrayBufferView in crypto
26025de 
PR-URL: #17595
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@apapirovski apapirovski apapirovski 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. fast-track PRs that do not need to wait for 48 hours to land.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.