Clarify Buffer hex string error message by jgeewax · Pull Request #4877 · nodejs/node

jgeewax · 2016-01-26T11:42:25Z

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).

bnoordhuis · 2016-01-26T16:39:18Z

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.

jasnell · 2016-01-27T21:09:37Z

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

jasnell · 2016-01-27T21:09:58Z

Marking semver-major because of the error message change.

jgeewax · 2016-01-28T19:00:34Z

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

cjihrig · 2016-01-28T21:51:46Z

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

cjihrig · 2016-01-28T21:53:16Z

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

Yes, this should be fine.

jgeewax · 2016-01-28T22:54:02Z

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

cjihrig · 2016-01-28T23:12:32Z

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 · 2016-01-28T23:14:09Z

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

jgeewax · 2016-01-28T23:14:28Z

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

cjihrig · 2016-01-28T23:17:30Z

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

jgeewax · 2016-01-28T23:26:40Z

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

trevnorris · 2016-01-30T20:25:23Z

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

jasnell · 2016-02-01T16:38:41Z

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

jasnell · 2016-02-01T16:39:38Z

~~LGTM with a couple of nits~~

jasnell · 2016-03-22T00:22:19Z

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

jgeewax · 2016-03-27T16:14:28Z

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

jasnell · 2016-03-27T16:17:02Z

~~LGTM~~

jasnell · 2016-04-02T03:57:30Z

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

silverwind added the buffer Issues and PRs related to the buffer subsystem. label Jan 26, 2016

mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 26, 2016

jasnell reviewed Jan 27, 2016
View reviewed changes

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

jgeewax force-pushed the patch-1 branch from c6520c8 to 70bc484Compare January 28, 2016 18:59

cjihrig reviewed Jan 28, 2016
View reviewed changes

jgeewax force-pushed the patch-1 branch from 70bc484 to 20e7082Compare January 28, 2016 22:53

jgeewax force-pushed the patch-1 branch from 20e7082 to 64cce87Compare January 28, 2016 23:14

jgeewax force-pushed the patch-1 branch from 64cce87 to 04f08afCompare January 28, 2016 23:26

jasnell reviewed Feb 1, 2016
View reviewed changes

jgeewax force-pushed the patch-1 branch 2 times, most recently from be3d46b to df3fe4fCompare March 27, 2016 16:14

jasnell reviewed Apr 2, 2016
View reviewed changes

Comment threadtest/parallel/test-buffer.js Outdated
jasnellApr 2, 2016
Copy link
Copy Markdown
Member
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')

snyk-ioBot mentioned this pull request Oct 21, 2024
[Snyk] Fix for 26 vulnerabilities Graybar-codespace/node#49
Open

Uh oh!

Uh oh!

Conversation

jgeewax commented Jan 26, 2016

Uh oh!

bnoordhuis commented Jan 26, 2016

Uh oh!

jasnellJan 27, 2016

Choose a reason for hiding this comment

Uh oh!

jgeewaxJan 28, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jan 27, 2016

Uh oh!

jgeewax commented Jan 28, 2016

Uh oh!

cjihrig commented Jan 28, 2016

Uh oh!

cjihrigJan 28, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuisJan 28, 2016

Choose a reason for hiding this comment

Uh oh!

jgeewax commented Jan 28, 2016

Uh oh!

cjihrig commented Jan 28, 2016

Uh oh!

cjihrig commented Jan 28, 2016

Uh oh!

jgeewax commented Jan 28, 2016

Uh oh!

cjihrig commented Jan 28, 2016

Uh oh!

jgeewax commented Jan 28, 2016

Uh oh!

trevnorris commented Jan 30, 2016

Uh oh!

jasnellFeb 1, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented Feb 1, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Mar 22, 2016

Uh oh!

jgeewax commented Mar 27, 2016

Uh oh!

jasnell commented Mar 27, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnellApr 2, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

jasnell commented Feb 1, 2016•
edited
Loading

jasnell commented Mar 27, 2016•
edited
Loading