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: Buffer() text copyediting #19567

Closed
wants to merge 3 commits into from
Closed

doc: Buffer() text copyediting #19567

wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 23, 2018

Rewording, punctuation, consistent sentence structure and italics, wrap
section at 80 characters.

Fix run-on sentence in buffer.md

Change v6 to 6.0.0. We abandoned v-notation for versions to avoid
confusion between v8 (version 8.0.0) and V8 (the JavaScript engine).

Checklist

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Mar 23, 2018
@Trott
Copy link
Member Author

Trott commented Mar 23, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/302/

vsemozhetbyt
vsemozhetbyt reviewed Mar 23, 2018
View reviewed changes
doc/api/buffer.md Outdated
appropriately initialize newly allocated `Buffer` content, can inadvertently
introduce security and reliability issues into their code.
Because the behavior of `new Buffer()` is different depending on the type of the
first argument, security and reliability issues can be inadvertantly introduced
Copy link
Contributor

Choose a reason for hiding this comment

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

inadvertantly -> inadvertently?

vsemozhetbyt
vsemozhetbyt reviewed Mar 23, 2018
View reviewed changes
doc/api/buffer.md Outdated
* [`Buffer.from(buffer)`] returns a new `Buffer` containing a *copy* of the
* [`Buffer.from(array)`] returns a new `Buffer` that *contains a copy* of the
provided octets.
* [`Buffer.from(arrayBuffer[, byteOffset [, length]])`]
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Mar 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This line break makes the link not to be rendered properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still the issue)

vsemozhetbyt
vsemozhetbyt reviewed Mar 23, 2018
View reviewed changes
doc/api/buffer.md Outdated
* [`Buffer.allocUnsafe(size)`][`Buffer.allocUnsafe()`] and
[`Buffer.allocUnsafeSlow(size)`][`Buffer.allocUnsafeSlow()`] each return a
new `Buffer` of the specified `size` whose content *must* be initialized
using either [`buf.fill(0)`][`buf.fill()`] or written to completely.
new unintialized `Buffer` of the specified `size`. Because the `Buffer` is
Copy link
Contributor

Choose a reason for hiding this comment

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

unintialized -> uninitialized?

vsemozhetbyt
vsemozhetbyt reviewed Mar 23, 2018
View reviewed changes
doc/api/buffer.md Outdated
new `Buffer` of the specified `size` whose content *must* be initialized
using either [`buf.fill(0)`][`buf.fill()`] or written to completely.
new unintialized `Buffer` of the specified `size`. Because the `Buffer` is
uninitalized, it may contain potentially-sensitive data. Do not use this
Copy link
Contributor

Choose a reason for hiding this comment

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

uninitalized -> uninitialized?

vsemozhetbyt
vsemozhetbyt approved these changes Mar 23, 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.

With nits)

@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 23, 2018
@Trott
Copy link
Member Author

Trott commented Mar 23, 2018

Guess I forgot to run the spell-check this time.....

@Trott Trott force-pushed the buffer-consistent-returns branch 4 times, most recently from 97692c8 to 0c8b45c Compare March 23, 2018 23:26
@Trott
Copy link
Member Author

Trott commented Mar 23, 2018

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/303/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 23, 2018
trivikr
trivikr approved these changes Mar 24, 2018
View reviewed changes
doc/api/buffer.md Outdated
* [`Buffer.alloc(size[, fill[, encoding]])`][`Buffer.alloc()`] returns a new
initialized `Buffer` of the specified size. This method is slower than
[`Buffer.allocUnsafe(size)`][`Buffer.allocUnsafe()`] but guarantees that newly
created `Buffer` instances never contain old and potentially-sensitive data.
Copy link
Member

Choose a reason for hiding this comment

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

nit: potentially and sensitive should be two different words?

Ditto on line 99

jasnell
jasnell requested changes Mar 24, 2018
View reviewed changes
doc/api/buffer.md Outdated
using either [`buf.fill(0)`][`buf.fill()`] or written to completely.
new uninitialized `Buffer` of the specified `size`. Because the `Buffer` is
uninitialized, it may contain potentially-sensitive data. Do not use this
method unless you know what you are doing.
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid you in the docs I would just strike this last sentence.

@Trott
doc: change v-notation for version in buffer.md
d8cda17 
Change v6 to 6.0.0. We abandoned v-notation for versions to avoid
confusion between v8 (version 8.0.0) and V8 (the JavaScript engine).
@Trott
doc: fix run-on sentence in buffer.md
8c21272
@Trott
doc: improve Buffer() text
8c184d8 
Rewording, punctuation, consistent sentence structure and italics, wrap
section at 80 characters.
@Trott Trott force-pushed the buffer-consistent-returns branch from 0c8b45c to 8c184d8 Compare March 24, 2018 21:51
@Trott
Copy link
Member Author

Trott commented Mar 24, 2018

Nits addressed.

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/316/

gibfahn
gibfahn approved these changes Mar 26, 2018
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

Is it worth saying that initialized means zero-filled?

@Trott
Copy link
Member Author

Trott commented Mar 26, 2018

@jasnell Is this good by you now that the word "you" is gone?

@Trott
Copy link
Member Author

Trott commented Mar 26, 2018

Is it worth saying that initialized means zero-filled?

Yes, except that it is not necessarily zero-filled, but filled with the value of fill which defaults to 0. I'll put that in a separate PR (so any bike-shedding on the wording doesn't delay these other changes) unless someone beats me to it.

Trott added a commit to Trott/io.js that referenced this pull request Mar 26, 2018
@Trott
doc: change v-notation for version in buffer.md
400faf5 
Change v6 to 6.0.0. We abandoned v-notation for versions to avoid
confusion between v8 (version 8.0.0) and V8 (the JavaScript engine).

PR-URL: nodejs#19567
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Mar 26, 2018
@Trott
doc: fix run-on sentence in buffer.md
ae120db 
PR-URL: nodejs#19567
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Mar 26, 2018
@Trott
doc: improve Buffer() text
d74919c 
Rewording, punctuation, consistent sentence structure and italics, wrap
section at 80 characters.

PR-URL: nodejs#19567
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@Trott
Copy link
Member Author

Trott commented Mar 26, 2018

Landed in 79fa372...d74919c

@Trott Trott closed this Mar 26, 2018
targos pushed a commit that referenced this pull request Mar 27, 2018
@Trott @targos
doc: change v-notation for version in buffer.md
962c581 
Change v6 to 6.0.0. We abandoned v-notation for versions to avoid
confusion between v8 (version 8.0.0) and V8 (the JavaScript engine).

PR-URL: #19567
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
targos pushed a commit that referenced this pull request Mar 27, 2018
@Trott @targos
doc: fix run-on sentence in buffer.md
2fadc9e 
PR-URL: #19567
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
targos pushed a commit that referenced this pull request Mar 27, 2018
@Trott @targos
doc: improve Buffer() text
8191ada 
Rewording, punctuation, consistent sentence structure and italics, wrap
section at 80 characters.

PR-URL: #19567
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@Trott Trott deleted the buffer-consistent-returns branch January 13, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@trivikr trivikr trivikr approved these changes

@gibfahn gibfahn gibfahn approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer 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.
7 participants
@Trott @jasnell @lpinca @vsemozhetbyt @gibfahn @trivikr @nodejs-github-bot