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: fix backwards incompatibility #12439

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@mscdex
Contributor

mscdex commented Apr 16, 2017

4a86803 introduced a backwards incompatibility by accident and was not caught due to an existing test that wasn't strict enough.

This commit fixes both the backwards incompatibility and the test.

This changes does not affect performance.

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

/cc @Trott

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • buffer
buffer: fix backwards incompatibility
4a86803 introduced a backwards incompatibility by accident
and was not caught due to an existing test that wasn't strict enough.

This commit fixes both the backwards incompatibility and the test.
@cjihrig

LGTM. Can the parens in lib/buffer.js be dropped though?

@ghost

ghost approved these changes Apr 16, 2017

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Apr 16, 2017

Member

I'm not opposed to this, but I want to suggest the possibility that we only want this on v7.x and earlier. I'm not sure it makes a lot of sense to have two different error messages for an invalid encoding, so the accidental removal of one of them in 4a86803 arguably makes the error messages better/more consistent and we might as well do it in 8.x.x and forward.

I say "arguably" because you could make the case that the one that specifically says "string" is better, and I suspect someone might make that argument. :-D

Member

Trott commented Apr 16, 2017

I'm not opposed to this, but I want to suggest the possibility that we only want this on v7.x and earlier. I'm not sure it makes a lot of sense to have two different error messages for an invalid encoding, so the accidental removal of one of them in 4a86803 arguably makes the error messages better/more consistent and we might as well do it in 8.x.x and forward.

I say "arguably" because you could make the case that the one that specifically says "string" is better, and I suspect someone might make that argument. :-D

@Trott

Trott approved these changes Apr 16, 2017

LGTM if CI is green.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Apr 16, 2017

Contributor

@cjihrig I prefer to include parens when using conditionals outside of an if, while, for, etc. so that they're easier to spot visually and the intention is clear. Also it's consistent with the style of the code that was already merged.

Contributor

mscdex commented Apr 16, 2017

@cjihrig I prefer to include parens when using conditionals outside of an if, while, for, etc. so that they're easier to spot visually and the intention is clear. Also it's consistent with the style of the code that was already merged.

jasnell added a commit that referenced this pull request Apr 18, 2017

buffer: fix backwards incompatibility
4a86803 introduced a backwards incompatibility by accident
and was not caught due to an existing test that wasn't strict enough.

This commit fixes both the backwards incompatibility and the test.

PR-URL: #12439
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Apr 18, 2017

Member

Landed in 7cd0d4f

Member

jasnell commented Apr 18, 2017

Landed in 7cd0d4f

@jasnell jasnell closed this Apr 18, 2017

@mscdex mscdex deleted the mscdex:buffer-perf-fix-backwards-compat branch May 15, 2017

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jun 18, 2017

Member

Depends on #12361.

Member

gibfahn commented Jun 18, 2017

Depends on #12361.

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