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: show symbol properties #247

Closed
wants to merge 1 commit into
base: v1.x
from

Conversation

Projects
None yet
3 participants
@vkurchatkin
Copy link
Member

vkurchatkin commented Jan 7, 2015

Properties with symbol names are shown if option showHidden of util.inspect or console.dir is true.

@bnoordhuis

View changes

lib/util.js Outdated
if (isSymbol(key)) {
name = '[' + ctx.stylize(key.toString(), 'symbol') + ']';
} else {
name = '[' + key.toString() + ']';

This comment has been minimized.

@bnoordhuis

bnoordhuis Jan 7, 2015

Member

I'm not 100% sure that you can assume that key is not null or undefined here.

This comment has been minimized.

@vkurchatkin

vkurchatkin Jan 7, 2015

Member

Well, a key could be either a symbol or a string. Anyway, removed toString, it's not needed and wasn't there in the first place. Not sure why symbols toString is not called automatically.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Jan 7, 2015

LGTM apart from one thing that is somewhere between a nit and a question.

util: show symbol properties
Properties with symbol names are shown if option `showHidden` of `util.inspect`
or `console.dir` is `true`.

@vkurchatkin vkurchatkin force-pushed the vkurchatkin:format-symbol-keys branch to 2502374 Jan 7, 2015

@vkurchatkin

This comment has been minimized.

Copy link
Member

vkurchatkin commented Jan 7, 2015

fixed and rebased

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Jan 7, 2015

LGTM. Can I have one more sign-off from another committer?

@rvagg

This comment has been minimized.

Copy link
Member

rvagg commented Jan 8, 2015

nice, LGTM

bnoordhuis added a commit that referenced this pull request Jan 8, 2015

util: show symbol properties
Properties with symbol names are shown if option `showHidden` of `util.inspect`
or `console.dir` is `true`.

PR-URL: #247
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Jan 8, 2015

Thanks Vladimir, landed in c70d192!

@bnoordhuis bnoordhuis closed this Jan 8, 2015

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