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: list encodings supported by buffer.transcode #22263

Closed
wants to merge 2 commits into from
Closed

doc: list encodings supported by buffer.transcode #22263

wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 11, 2018

Fixes: #15632

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@jasnell jasnell added 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. labels Aug 11, 2018
@nodejs-github-bot
Copy link
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/518/pipeline

@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 Aug 11, 2018
@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2018
@Trott Trott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2018
trivikr
trivikr reviewed Aug 11, 2018
View reviewed changes
doc/api/buffer.md Outdated
@@ -2508,6 +2508,9 @@ encoding to another. Returns a new `Buffer` instance.
Throws if the `fromEnc` or `toEnc` specify invalid character encodings or if
conversion from `fromEnc` to `toEnc` is not permitted.

Encodings supported by `buffer.transcode()` include: `'ascii'`, `'utf8'`,
`'utf16le'`, `'ucs2'`, `'latin1'`, and `'binary'`.
Copy link
Member

Choose a reason for hiding this comment

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

Does 'hex' needs to be added here? (documentation)

Copy link
Member Author

Choose a reason for hiding this comment

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

No. 'hex' is not supported

Trott
Trott reviewed Aug 11, 2018
View reviewed changes
doc/api/buffer.md Outdated
@@ -2508,6 +2508,9 @@ encoding to another. Returns a new `Buffer` instance.
Throws if the `fromEnc` or `toEnc` specify invalid character encodings or if
conversion from `fromEnc` to `toEnc` is not permitted.

Encodings supported by `buffer.transcode()` include: `'ascii'`, `'utf8'`,
Copy link
Member

Choose a reason for hiding this comment

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

include -> are assuming the list is supposed to be a complete list of supported encodings.

Copy link
Member Author

@jasnell jasnell Aug 11, 2018 •
edited

Choose a reason for hiding this comment

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

Feel free to make whatever editorial changes you'd like

Copy link
Member

Choose a reason for hiding this comment

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

Wanted to confirm the list is supposed to be complete before doing it. I'll take that as a "yes, it's a complete list" and make the change now. 👍

@Trott
Copy link
Member

Trott commented Aug 11, 2018

@nodejs/documentation Not sure if there's a desired formatting standard for "string values permitted for this argument must be from this small set". I don't think we have one, so I'm fine with this the way it is. Regardless, PTAL.

@Trott
Copy link
Member

Trott commented Aug 11, 2018

@nodejs/buffer

@vsemozhetbyt
Copy link
Contributor

Node.js Collaborators, please, add 👍 here if you approve fast-tracking.

jasnell added a commit that referenced this pull request Aug 12, 2018
@jasnell
doc: list encodings supported by buffer.transcode
a5b3c15 
Fixes: #15632

PR-URL: #22263
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Aug 12, 2018

Landed in a5b3c15 (fast tracked with approval)

@jasnell jasnell closed this Aug 12, 2018
targos pushed a commit that referenced this pull request Aug 12, 2018
@jasnell @targos
doc: list encodings supported by buffer.transcode
ab1e5d1 
Fixes: #15632

PR-URL: #22263
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@rvagg rvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
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.

None yet
7 participants
@jasnell @nodejs-github-bot @Trott @vsemozhetbyt @lpinca @cjihrig @trivikr