Skip to content

Conversation

@jasnell
Copy link
Member

Fixes: #15632

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

@jasnelljasnell 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

@nodejs-github-botnodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations. labels Aug 11, 2018
@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2018
@TrottTrott removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 11, 2018
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
MemberAuthor

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

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
MemberAuthor

@jasnelljasnellAug 11, 2018

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

@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

@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
Fixes: #15632 PR-URL: #22263 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
MemberAuthor

Landed in a5b3c15 (fast tracked with approval)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bufferIssues and PRs related to the buffer subsystem.docIssues and PRs related to the documentations.fast-trackPRs that do not need to wait for 48 hours to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@jasnell@nodejs-github-bot@Trott@vsemozhetbyt@lpinca@cjihrig@trivikr