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

Buffer bug fix #6775

Closed
wants to merge 6 commits into from
Closed

Buffer bug fix #6775

wants to merge 6 commits into from

Conversation

jspri
Copy link
Contributor

@jspri jspri commented May 15, 2016

Checklist
  • tests and code linting passes - Linting passes, tests had failures but none additional to master
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

Buffer

Description of change

Fixes #6770, Single digit hex strings not raising TypeError on Buffer creation

@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label May 15, 2016
@jasnell
Copy link
Member

jasnell commented May 15, 2016

@nodejs/buffer

@@ -749,6 +749,11 @@ for (let i = 0; i < 256; i++) {
assert.equal(hexb2[i], hexb[i]);
}

//#6770 Test single hex character throws TypeError
Copy link
Member

Choose a reason for hiding this comment

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

You can reference github issues in comments, but it is usually preferred to give the full URL for things like that so that it’s easier to look up and there is no ambiguity wrt the old issue tracker at https://github.com/nodejs/node-v0.x-archive/

@thefourtheye
Copy link
Contributor

Slightly related #3773

@jspri
Copy link
Contributor Author

jspri commented May 18, 2016

Should be good to go I think

}, TypeError);

// Test single base64 char encodes as 0
assert.equal(Buffer.from('A', 'base64'), 0);
Copy link
Member

Choose a reason for hiding this comment

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

This should use assert.strictEqual and should test the length of the buffer rather than the buffer itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed now

@addaleax
Copy link
Member

LGTM… CI: https://ci.nodejs.org/job/node-test-commit/3437/

@Crazometer btw, Github doesn’t send out notifications when somebody pushes new commits to a PR, so posting a quick comment helps to make sure noething gets lost :)

@jspri
Copy link
Contributor Author

jspri commented May 23, 2016

@addaleax Thanks for that, you could read my mind

@addaleax
Copy link
Member

CI is green.

You author name in this commit is given as “Crazometer”. Is that intended or do you prefer to be listed (changelog, git log, AUTHORS file) with some other name? People typically prefer their full name, but ultimately it’s up to you.

@jspri
Copy link
Contributor Author

jspri commented May 23, 2016

"Justin Sprigg" if you could thanks. I'll make sure to change that next time.

addaleax pushed a commit that referenced this pull request May 23, 2016
Fixes single digit hex strings not raising TypeError on Buffer creation.

Fixes: #6770
PR-URL: #6775
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax
Copy link
Member

Landed in 05e2acb. Thanks for fixing this!

@addaleax addaleax closed this May 23, 2016
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
Fixes single digit hex strings not raising TypeError on Buffer creation.

Fixes: #6770
PR-URL: #6775
Reviewed-By: Anna Henningsen <[email protected]>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
Fixes single digit hex strings not raising TypeError on Buffer creation.

Fixes: #6770
PR-URL: #6775
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins
Copy link
Contributor

I'm assuming this doesn't affect the v4.x buffer system

/cc @jasnell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants