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.

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

doc: improve 'hex' Buffer decoding description and examples #41598

Merged
merged 4 commits into from
Jan 21, 2022

Conversation

gioragutt
Copy link
Contributor

@gioragutt gioragutt commented Jan 19, 2022

Fixes: #41594

@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 Jan 19, 2022
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Please run the linter 🙏 ( the word 'hexadecimal' needs to drop a line)

doc/api/buffer.md Outdated Show resolved Hide resolved
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jan 19, 2022
may occur when decoding strings that do exclusively contain valid hexadecimal
characters. See below for an example.
may occur when decoding strings that do not exclusively contain valid
hexadecimal characters. See below for an example.
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, it doesn't matter much if the strings exclusively contain hexadecimal characters. In fact, it might be more surprising that Buffer.from truncates data even when it only contains hex digits:

// only hex, still truncated :(
Buffer.from('000', 'hex').toString('hex') === '00'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it because it reads 2 characters at a time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nvm, read the docs, which state:

Buffer.from('1a7g', 'hex');
// Prints <Buffer 1a>, data truncated when data ends in single digit ('7').

So the last '0' is truncated because it's a single character, and the hex decoder expects pairs of characters.
So since the third '0' does not have a pairing character, it is truncated.

Perhaps the description should state that? You know, not only in the code example?

@aduh95 aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 19, 2022
Comment on lines 178 to 179
may occur when decoding strings that do not exclusively contain valid
hexadecimal characters. See below for an example.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
may occur when decoding strings that do not exclusively contain valid
hexadecimal characters. See below for an example.
may occur when decoding strings that do not exclusively consist of an even
number of hexadecimal characters. See below for an example.

Maybe something like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that this, plus another code example (something like 123 -> 12) would be very nice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tniessen WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

SGTM. These are unfortunate oddities that we need to maintain for backward compatibility :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a better look at the examples and figured that I could achieve the same thing by altering the strings used in the examples.

@Mesteery Mesteery added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 19, 2022
Comment on lines 177 to +179
* `'hex'`: Encode each byte as two hexadecimal characters. Data truncation
may occur when decoding strings that do exclusively contain valid hexadecimal
characters. See below for an example.
may occur when decoding strings that do not exclusively consist of an even
number of hexadecimal characters. See below for an example.
Copy link
Member

Choose a reason for hiding this comment

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

Optional nit-pick suggestion: The Microsoft style guide we follow suggests might instead of may in this context. But I wonder if it's actually will occur rather than might occur? All told:

Suggested change
* `'hex'`: Encode each byte as two hexadecimal characters. Data truncation
may occur when decoding strings that do exclusively contain valid hexadecimal
characters. See below for an example.
may occur when decoding strings that do not exclusively consist of an even
number of hexadecimal characters. See below for an example.
* `'hex'`: Encode each byte as two hexadecimal characters. Data is truncated
when decoding strings that do not exclusively consist of an even
number of hexadecimal characters. See below for an example.

@gioragutt gioragutt changed the title doc: add missing word in 'hex' Buffer decoding description doc: improve 'hex' Buffer decoding description and examples Jan 20, 2022
@aduh95 aduh95 merged commit dbe60a2 into nodejs:master Jan 21, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jan 21, 2022

Landed in dbe60a2

aduh95 pushed a commit to aduh95/node that referenced this pull request Jan 22, 2022
fixes nodejs#41594

PR-URL: nodejs#41598
Fixes: nodejs#41594
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@gioragutt gioragutt deleted the patch-1 branch January 22, 2022 11:14
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
PR-URL: #41598
Fixes: #41594
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
PR-URL: nodejs#41598
Fixes: nodejs#41594
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 28, 2022
PR-URL: #41598
Fixes: #41594
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
PR-URL: #41598
Fixes: #41594
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
PR-URL: #41598
Fixes: #41594
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
PR-URL: #41598
Fixes: #41594
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Mestery <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buffer hex encoding description seems to omit a word
9 participants