Skip to content

Clarify Buffer hex string error message #4877

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
Closed
wants to merge 1 commit into from
Closed

Clarify Buffer hex string error message #4877

wants to merge 1 commit into from

Conversation

@jgeewax
Copy link

@jgeewax jgeewax commented Jan 26, 2016

Taking a clue from https://github.com/nodejs/node/blob/master/src/string_bytes.cc#L503, provide more reason for why a hex string is invalid (namely that the issue is the length being odd rather than even).

@silverwind silverwind added the buffer Issues and PRs related to the buffer subsystem. label Jan 26, 2016
@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 26, 2016

I don't think we have a regression test for this. Can you add one to test/parallel/test-buffer.js?

Apropos the commit log, can you make sure it conforms to the guidelines from CONTRIBUTING.md? Thanks.

@mscdex mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 26, 2016
jasnell
jasnell reviewed Jan 27, 2016
View reviewed changes
src/node_buffer.cc Outdated
Copy link
Member

@jasnell jasnell Jan 27, 2016

Choose a reason for hiding this comment

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

Perhaps, "The hex string length must be an even number" or "The hex string must have an even number of characters"

Copy link
Author

@jgeewax jgeewax Jan 28, 2016

Choose a reason for hiding this comment

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

Done :)

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 27, 2016
@jasnell
Copy link
Member

jasnell commented Jan 27, 2016

Marking semver-major because of the error message change.

@jgeewax
Copy link
Author

jgeewax commented Jan 28, 2016

@bnoordhuis I added a simple test that looks for the TypeError and (new) message.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 28, 2016

Can you run make lint on this, and correct the errors.

cjihrig
cjihrig reviewed Jan 28, 2016
View reviewed changes
src/node_buffer.cc
Copy link
Contributor

@cjihrig cjihrig Jan 28, 2016

Choose a reason for hiding this comment

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

@bnoordhuis does our C++ style allow strings split like this (the linter doesn't complain)?

Copy link
Member

@bnoordhuis bnoordhuis Jan 28, 2016

Choose a reason for hiding this comment

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

Yes, this should be fine.

@jgeewax
Copy link
Author

jgeewax commented Jan 28, 2016

@cjihrig Did a make lint and fixed anything outstanding. Let me know if you need anything else

@cjihrig
Copy link
Contributor

cjihrig commented Jan 28, 2016

The commit message should be buffer:, not buffers:, but whoever lands the commit can fix that up.

LGTM, but maybe @trevnorris wants to sign off on this?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 28, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/1423/

@jgeewax
Copy link
Author

jgeewax commented Jan 28, 2016

@cjihrig : Fixed. I was going off the commit log, which seemed to have both. Sorry about that.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 28, 2016

Looks like a commit just landed that conflicts with this one. Mind rebasing?

@jgeewax
Copy link
Author

jgeewax commented Jan 28, 2016

No problem. Let me know if that works for you.

@trevnorris
Copy link
Contributor

trevnorris commented Jan 30, 2016

Is this the only location we throw for incorrect hex string length? If so then LGTM.

jasnell
jasnell reviewed Feb 1, 2016
View reviewed changes
test/parallel/test-buffer.js Outdated
Copy link
Member

@jasnell jasnell Feb 1, 2016

Choose a reason for hiding this comment

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

minor nit: /^The hex string must have an even number of characters/.test(err.message)

@jasnell
Copy link
Member

jasnell commented Feb 1, 2016 •
edited
Loading

LGTM with a couple of nits

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

@jgeewax ... ping. There were a few nits that need to be looked at.

@jgeewax jgeewax force-pushed the patch-1 branch 2 times, most recently from be3d46b to df3fe4f Compare March 27, 2016 16:14
@jgeewax
Copy link
Author

jgeewax commented Mar 27, 2016

OK -- Addressed the console.log stuff as well as the nit for regex checking vs equality.

@jasnell
Copy link
Member

jasnell commented Mar 27, 2016 •
edited
Loading

LGTM

jasnell
jasnell reviewed Apr 2, 2016
View reviewed changes
test/parallel/test-buffer.js Outdated
Copy link
Member

@jasnell jasnell Apr 2, 2016

Choose a reason for hiding this comment

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

minor nit: can you update this to use Buffer.from('8', 'hex')

This was referenced Jul 7, 2023
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Merged
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
@snyk-io snyk-io bot mentioned this pull request Oct 21, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuis bnoordhuis bnoordhuis requested changes

@cjihrig cjihrig cjihrig requested changes

@fhinkel fhinkel fhinkel approved these changes

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.