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

util: inspect boxed symbols like other primitives #7641

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jul 9, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

util

Description of change

Inspect boxed symbol objects in the same way other boxed primitives are inspected.

Fixes: #7639

CI: https://ci.nodejs.org/job/node-test-commit/4039/

Inspect boxed symbol objects in the same way other boxed primitives
are inspected.

Fixes: nodejs#7639
@addaleax addaleax added the util Issues and PRs related to the built-in util module. label Jul 9, 2016
@targos
Copy link
Member

targos commented Jul 10, 2016

LGTM

3 similar comments
@bnoordhuis
Copy link
Member

LGTM

@benjamingr
Copy link
Member

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Jul 11, 2016

LGTM

@@ -474,6 +474,7 @@ test_lines({

// test boxed primitives output the correct values
assert.equal(util.inspect(new String('test')), '[String: \'test\']');
assert.equal(util.inspect(Object(Symbol('test'))), '[Symbol: Symbol(test)]');
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this look like having redundant information?

Copy link
Member

Choose a reason for hiding this comment

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

No, IMO. It conveys that it's a symbol wrapped in an object; i.e., that typeof object === 'object' but that typeof object.valueOf() === 'symbol'.

Copy link
Member Author

Choose a reason for hiding this comment

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

@thefourtheye I think learning the difference between Symbol(test) and [Symbol: test] might be a bit harder for people new to symbols.

This way might not be perfect, but I think the consistency with how other boxed values are displayed makes it easier to reason about what exactly is being displayed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. LGTM.

@addaleax
Copy link
Member Author

Landed in 35e8c94

@addaleax addaleax closed this Jul 17, 2016
@addaleax addaleax deleted the util-inspect-symbol branch July 17, 2016 16:11
addaleax added a commit that referenced this pull request Jul 17, 2016
Inspect boxed symbol objects in the same way other boxed primitives
are inspected.

Fixes: #7639
PR-URL: #7641
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
evanlucas pushed a commit that referenced this pull request Jul 19, 2016
Inspect boxed symbol objects in the same way other boxed primitives
are inspected.

Fixes: #7639
PR-URL: #7641
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
evanlucas pushed a commit that referenced this pull request Jul 20, 2016
Inspect boxed symbol objects in the same way other boxed primitives
are inspected.

Fixes: #7639
PR-URL: #7641
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
evanlucas added a commit that referenced this pull request Jul 21, 2016
Notable changes:

* **buffer**:
  * Improve performance of Buffer.from(str, 'hex') and Buffer#write(str, 'hex'). (Christopher Jeffrey) #7602
  * Fix creating from zero-length ArrayBuffer. (Ingvar Stepanyan) #7176
* **deps**: Upgrade to V8 5.0.71.xx. (Ben Noordhuis) #7531
* **repl**: Fix issue with function redeclaration. (Prince J Wesley) #7794
* **util**: Fix inspecting of boxed symbols. (Anna Henningsen) #7641

PR-URL: #7782
evanlucas added a commit that referenced this pull request Jul 21, 2016
Notable changes:

* **buffer**:
  * Improve performance of Buffer.from(str, 'hex') and Buffer#write(str, 'hex'). (Christopher Jeffrey) #7602
  * Fix creating from zero-length ArrayBuffer. (Ingvar Stepanyan) #7176
* **deps**:
  * Upgrade to V8 5.0.71.xx. (Ben Noordhuis) #7531
  * Backport V8 instanceof bugfix (Franziska Hinkelmann) #7638
* **repl**: Fix issue with function redeclaration. (Prince J Wesley) #7794
* **util**: Fix inspecting of boxed symbols. (Anna Henningsen) #7641

PR-URL: #7782
evanlucas added a commit that referenced this pull request Jul 21, 2016
Notable changes:

* **buffer**:
  * Improve performance of Buffer.from(str, 'hex') and Buffer#write(str, 'hex'). (Christopher Jeffrey) #7602
  * Fix creating from zero-length ArrayBuffer. (Ingvar Stepanyan) #7176
* **deps**:
  * Upgrade to V8 5.0.71.xx. (Ben Noordhuis) #7531
  * Backport V8 instanceof bugfix (Franziska Hinkelmann) #7638
* **repl**: Fix issue with function redeclaration. (Prince J Wesley) #7794
* **util**: Fix inspecting of boxed symbols. (Anna Henningsen) #7641

PR-URL: #7782
lukesampson pushed a commit to ScoopInstaller/Scoop that referenced this pull request Jul 24, 2016
### Notable changes

* **buffer**:
  * Improve performance of Buffer.from(str, 'hex') and Buffer#write(str, 'hex'). (Christopher Jeffrey) [#7602](nodejs/node#7602)
  * Fix creating from zero-length ArrayBuffer. (Ingvar Stepanyan) [#7176](nodejs/node#7176)
* **deps**:
  * Upgrade to V8 5.0.71.xx. (Ben Noordhuis) [#7531](nodejs/node#7531)
  * Backport V8 instanceof bugfix (Franziska Hinkelmann) [#7638](nodejs/node#7638)
* **repl**: Fix issue with function redeclaration. (Prince J Wesley) [#7794](nodejs/node#7794)
* **util**: Fix inspecting of boxed symbols. (Anna Henningsen) [#7641](nodejs/node#7641)
MylesBorins pushed a commit that referenced this pull request Aug 30, 2016
Inspect boxed symbol objects in the same way other boxed primitives
are inspected.

Fixes: #7639
PR-URL: #7641
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2016
Inspect boxed symbol objects in the same way other boxed primitives
are inspected.

Fixes: #7639
PR-URL: #7641
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Inspect boxed symbol objects in the same way other boxed primitives
are inspected.

Fixes: #7639
PR-URL: #7641
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Inspect boxed symbol objects in the same way other boxed primitives
are inspected.

Fixes: #7639
PR-URL: #7641
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants