Skip to content

doc: shorten character encoding introduction#19648

Closed
Trott wants to merge 2 commits intonodejs:masterfrom
Trott:char-encodings
Closed

doc: shorten character encoding introduction#19648
Trott wants to merge 2 commits intonodejs:masterfrom
Trott:char-encodings

Conversation

@Trott
Copy link
Member

@Trott Trott commented Mar 28, 2018

Keep the introduction for Buffers and character encodings short and to
the point. The current introduction doesn't provide much in the way of
useful additional information, but it is a bit confusing in its wording.
("such as" seems like it ought to refer to "encoded characters" but it
actually refers to character encodings, which are not mentioned in the
sentence. It may be arguable as to whether "hex-encoded" is in fact a
character encoding, whether it should be stylized as "Hex-encoded" or
not, and whether it should be spelled out as "Hexadecimal-encoded". None
of that information is particularly useful to the end user at this point
in the text. Omitting it simplifies and improves the documentation.)

Additionally, the section is now wrapped to 80 characters.

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 28, 2018
@Trott
Copy link
Member Author

Trott commented Mar 28, 2018

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

gireeshpunathil
gireeshpunathil previously requested changes Mar 28, 2018
View reviewed changes
Copy link
Member

@gireeshpunathil gireeshpunathil left a comment

Choose a reason for hiding this comment

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

sorry, but -1 for this change as such, it takes out information without providing alternatives IMO.

suggestions:

  • reword the removed text by addressing the problems you spotted, or stating the common use case for the character encodings.

  • the example provided does not fully cover the current wording (converting between string and buffer) instead only buffer to string. An example of Buffer.from(string[, encoding]) will make it complete.

@TimothyGu
Copy link
Member

TimothyGu commented Mar 28, 2018

I feel I would agree with @gireeshpunathil.

@Trott
Copy link
Member Author

Trott commented Mar 28, 2018

  • the example provided does not fully cover the current wording (converting between string and buffer) instead only buffer to string. An example of Buffer.from(string[, encoding]) will make it complete.

The first line of the example code is a Buffer.from() with an encoding. Perhaps the example can be made more prominent or include a Buffer.from() with a less typical encoding to draw attention to it?

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Mar 28, 2018

sure, that looks great, @Trott !

@Trott
doc: shorten character encoding introduction
305633e 
Keep the introduction for Buffers and character encodings short and to
the point. The current introduction doesn't provide much in the way of
useful additional information, but it is a bit confusing in its wording.
("such as" seems like it ought to refer to "encoded characters" but it
actually refers to character encodings, which are not mentioned in the
sentence. It may be arguable as to whether "hex-encoded" is in fact a
character encoding, whether it should be stylized as "Hex-encoded" or
not, and whether it should be spelled out as "Hexadecimal-encoded". None
of that information is particularly useful to the end user at this point
in the text. Omitting it simplifies and improves the documentation.)

Additionally, the section is now wrapped to 80 characters.
@Trott
Copy link
Member Author

Trott commented Mar 29, 2018

@gireeshpunathil @TimothyGu I've tried to address the comments. PTAL. Thanks.

@gireeshpunathil gireeshpunathil dismissed their stale review March 29, 2018 04:52

thanks @Trott, changes looks good to me now.

@Trott
Copy link
Member Author

Trott commented Mar 29, 2018

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 29, 2018
TimothyGu
TimothyGu approved these changes Mar 29, 2018
View reviewed changes
doc/api/buffer.md

console.log(Buffer.from('fhqwhgads', 'ascii'));
// Prints: <Buffer 66 68 71 77 68 67 61 64 73>
console.log(Buffer.from('fhqwhgads', 'ucs2'));
Copy link
Member

@TimothyGu TimothyGu Mar 29, 2018

Choose a reason for hiding this comment

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

I'd very slightly prefer the name 'utf16le', which is I think better known (and more accurate) than 'ucs2'.

@Trott
Copy link
Member Author

Trott commented Mar 29, 2018

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

Trott added a commit to Trott/io.js that referenced this pull request Mar 30, 2018
@Trott
doc: shorten character encoding introduction
6c5144f 
Keep the introduction for Buffers and character encodings short and to
the point. The current introduction doesn't provide much in the way of
useful additional information, but it is a bit confusing in its wording.
("such as" seems like it ought to refer to "encoded characters" but it
actually refers to character encodings, which are not mentioned in the
sentence. It may be arguable as to whether "hex-encoded" is in fact a
character encoding, whether it should be stylized as "Hex-encoded" or
not, and whether it should be spelled out as "Hexadecimal-encoded". None
of that information is particularly useful to the end user at this point
in the text. Omitting it simplifies and improves the documentation.)

Additionally, the section is now wrapped to 80 characters.

PR-URL: nodejs#19648
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@Trott
Copy link
Member Author

Trott commented Mar 30, 2018

Landed in 6c5144f

@Trott Trott closed this Mar 30, 2018
Trott added a commit to Trott/io.js that referenced this pull request Mar 30, 2018
@Trott
doc: favor utf16le over ucs2 in buffer.md
0cabe09 
Favor 'utf16le' over its alias 'ucs2' in `buffer.md`.

Ref: nodejs#19648 (comment)
@Trott Trott mentioned this pull request Mar 30, 2018
Closed
2 tasks
Trott added a commit to Trott/io.js that referenced this pull request Apr 2, 2018
@Trott
doc: favor utf16le over ucs2 in buffer.md
52a9f42 
Favor 'utf16le' over its alias 'ucs2' in `buffer.md`.

Ref: nodejs#19648 (comment)

PR-URL: nodejs#19688
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 2, 2018
@Trott @targos
doc: shorten character encoding introduction
9f20534 
Keep the introduction for Buffers and character encodings short and to
the point. The current introduction doesn't provide much in the way of
useful additional information, but it is a bit confusing in its wording.
("such as" seems like it ought to refer to "encoded characters" but it
actually refers to character encodings, which are not mentioned in the
sentence. It may be arguable as to whether "hex-encoded" is in fact a
character encoding, whether it should be stylized as "Hex-encoded" or
not, and whether it should be spelled out as "Hexadecimal-encoded". None
of that information is particularly useful to the end user at this point
in the text. Omitting it simplifies and improves the documentation.)

Additionally, the section is now wrapped to 80 characters.

PR-URL: #19648
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
targos pushed a commit that referenced this pull request Apr 2, 2018
@Trott @targos
doc: favor utf16le over ucs2 in buffer.md
d111037 
Favor 'utf16le' over its alias 'ucs2' in `buffer.md`.

Ref: #19648 (comment)

PR-URL: #19688
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Apr 4, 2018
Merged
@Trott Trott deleted the char-encodings 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

@jasnell jasnell jasnell approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

+1 more reviewer

@TimothyGu TimothyGu TimothyGu approved these changes

Reviewers whose approvals may not affect merge requirements

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Trott @TimothyGu @gireeshpunathil @jasnell @nodejs-github-bot