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: add tests for Buffer.prototype.inspect() and refactor #11600

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 28, 2017

First commit: lib/buffer.js defines Buffer.prototype.inspect() to override how buffers are presented by util.inspect(). Add basic tests for it. (This test completes code coverage for the function. Previously, the condition where the this.length > 0 was always true during test runs. This adds a test where it will be false.)

Second commit: Now that tests are in place, refactor. Replace toString().match().join() with toString().replace().trim(). This enables the elimination of a length check becuase replace() will return empty string if Buffer is empty whereas match() returns null. (To be clear, I don't think the .length check is a performance liability or anything like that. I just think that fewer logic branches usually makes for code that is easier to understand.)

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 test

@Trott Trott added buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests. labels Feb 28, 2017
@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Feb 28, 2017
@Trott
Copy link
Member Author

Trott commented Feb 28, 2017

lib/buffer.js Outdated
}
str = this.toString('hex', 0, max).replace(/(.{2})/g, '$1 ').trim();
if (this.length > max)
str += ' ... ';
return '<' + this.constructor.name + ' ' + str + '>';
Copy link
Member

Choose a reason for hiding this comment

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

nit: Template string here? ;)

lib/buffer.js defines Buffer.prototype.inspect() to override how buffers
are presented by util.inspect(). Add basic tests for it.
Replace toString().match().join() with toString().replace().trim(). This
enables the elimination of a length check becuase replace() will return
empty string if Buffer is empty whereas match() returns null.
@Trott
Copy link
Member Author

Trott commented Mar 2, 2017

Nit addressed. New CI: https://ci.nodejs.org/job/node-test-pull-request/6649/

jasnell pushed a commit that referenced this pull request Mar 2, 2017
lib/buffer.js defines Buffer.prototype.inspect() to override how buffers
are presented by util.inspect(). Add basic tests for it.

PR-URL: #11600
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 2, 2017
Replace toString().match().join() with toString().replace().trim(). This
enables the elimination of a length check becuase replace() will return
empty string if Buffer is empty whereas match() returns null.

PR-URL: #11600
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 2, 2017

Landed in bb5c84a...4c05d6a

@jasnell jasnell closed this Mar 2, 2017
addaleax pushed a commit that referenced this pull request Mar 5, 2017
lib/buffer.js defines Buffer.prototype.inspect() to override how buffers
are presented by util.inspect(). Add basic tests for it.

PR-URL: #11600
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit that referenced this pull request Mar 5, 2017
Replace toString().match().join() with toString().replace().trim(). This
enables the elimination of a length check becuase replace() will return
empty string if Buffer is empty whereas match() returns null.

PR-URL: #11600
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
lib/buffer.js defines Buffer.prototype.inspect() to override how buffers
are presented by util.inspect(). Add basic tests for it.

PR-URL: #11600
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
Replace toString().match().join() with toString().replace().trim(). This
enables the elimination of a length check becuase replace() will return
empty string if Buffer is empty whereas match() returns null.

PR-URL: #11600
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
lib/buffer.js defines Buffer.prototype.inspect() to override how buffers
are presented by util.inspect(). Add basic tests for it.

PR-URL: #11600
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
Replace toString().match().join() with toString().replace().trim(). This
enables the elimination of a length check becuase replace() will return
empty string if Buffer is empty whereas match() returns null.

PR-URL: #11600
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
lib/buffer.js defines Buffer.prototype.inspect() to override how buffers
are presented by util.inspect(). Add basic tests for it.

PR-URL: nodejs/node#11600
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
Replace toString().match().join() with toString().replace().trim(). This
enables the elimination of a length check becuase replace() will return
empty string if Buffer is empty whereas match() returns null.

PR-URL: nodejs/node#11600
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott Trott deleted the buffer-inspect branch January 13, 2022 22:34
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. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants