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: allow symbol-based custom inspection methods #8174

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

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

util

Description of change

Add a util.inspect.Custom Symbol which can be used to customize util.inspect() output. Providing obj[util.inspect.Custom] works like providing obj.inspect, except that the former allows avoiding name clashes with other inspect() methods.

Fixes: #8071

I’d appreciate feedback both on the symbol’s name and the way I changed the docs on custom inspection functions.

@addaleax addaleax added util Issues and PRs related to the built-in util module. semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 18, 2016
@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Aug 18, 2016
maybeCustomInspect !== exports.inspect &&
// Also filter out any prototype objects using the circular check.
!(value.constructor && value.constructor.prototype === value)) {
let ret = maybeCustomInspect.call(value, recurseTimes, ctx);
Copy link
Member

Choose a reason for hiding this comment

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

nit: looks like the indent is off here

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell This line is indented two spaces more than the opening if ( is, I think that’s the general rule. I know the code looks a bit weird, yes.

Copy link
Member

Choose a reason for hiding this comment

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

Alas, this is probably a result of our style choice of aligning the separate lines of the conditional.

At the risk of going off on a low-value irrelevant tangent: I have come to greatly dislike all the alignment stuff and wouldn't mind (at a minimum) removing the custom rules in ESLint that enforces it.

The main problems with alignment (from my perspective) are:

  • Does not allow for modification in situations like this where alignment (which can wind up at any column) diminishes ease of scanning and understanding due to clashing with fixed indentation. Fixed indentation is more important than alignment IMO and so we should just ditch alignment and use fixed indentation or (more likely, given the churn) no rules at all everywhere that we are using alignment.
  • As others have pointed out: Alignment looks great when you first do it. Then one line of the fourteen that are aligned gets changed, and you have to change all the others, resulting in more churn.

By the way, I'm pretty sure I wrote the custom rules that enforce alignment (after I got a nit or three about my failure to align stuff, probably), so I'll add: SORRY!!!

Copy link
Member

Choose a reason for hiding this comment

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

ok, I see it now... visually it looks off at first glance

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott For the most part I think the alignment rules are okay, I see no need to change that or apologize for them. ;)

What I like to do in cases like this is actually not changing the alignment, but pulling the opening { into its own line… I assume people here (and the linter) aren’t fans of that either, so it’s probably okay to leave it as it is right now. :)

Copy link
Member

Choose a reason for hiding this comment

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

@addaleax I like to put the first part of the conditional on its own line so that it gets fixed indentation, but I'm also not sure others are a fan of that:

if (
  somethingReallyLong &&
  somethingElseThatIsAlsoReallyLong &&
  oneMoreReallyLongThing
) {
  doSomething();
}

Copy link
Member Author

Choose a reason for hiding this comment

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

@Trott … yeah … makes me wonder if Python is the only lang that got it right ;)

I’m going to leave this as it is if everyone’s okay with that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, totally OK with that. Sorry for the distracting rant.

Copy link
Member

Choose a reason for hiding this comment

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

(Another rant for another time: Don't limit the line length to 80 characters?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, no need to apologize. :)

(Another rant for another time: Don't limit the line length to 80 characters?)

#holy-war ? ;)

@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

Generally LGTM, I'd add the example of using inspect() back into the docs then maybe include a statement that using the inspect() approach is no longer recommended and may become deprecated in the future.

@addaleax
Copy link
Member Author

Updated with nits addressed… CI: https://ci.nodejs.org/job/node-test-pull-request/3745/

I don’t know about recommending to no longer use inspect() – I doubt this is going to make it into v4.x, so anybody who wants to support v4 will need to stick to using inspect() for now or provide both methods, and there isn’t exactly something wrong with using inspect().

@jasnell
Copy link
Member

jasnell commented Aug 19, 2016

Ok, yep, you're right. No need to rush it

@targos
Copy link
Member

targos commented Aug 19, 2016

I find it weird that the Symbol's name starts with an uppercase. I don't know about other uses or best practice but it's not the case for well-known symbols like Symbol.iterator

@addaleax
Copy link
Member Author

I’m happy with an all-lowercase variant too, util.inspect.custom or something like that. Does anybody else have opinions?

@jasnell
Copy link
Member

jasnell commented Aug 22, 2016

Lowercase works for me

On Sunday, August 21, 2016, Anna Henningsen notifications@github.com
wrote:

I’m happy with an all-lowercase variant too, util.inspect.custom or
something like that. Does anybody else have opinions?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8174 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eRG7iU5PKxju3tGI4ztyUk-emQiaks5qiNx6gaJpZM4Jn_-o
.

@addaleax addaleax force-pushed the util-inspect-symbol2 branch 2 times, most recently from 3ef0b4c to 1124ead Compare August 22, 2016 07:51
@addaleax
Copy link
Member Author

Updated using util.inspect.custom, new CI: https://ci.nodejs.org/job/node-test-commit/4708/

@jasnell
Copy link
Member

jasnell commented Aug 22, 2016

LGTM

Add a `util.inspect.custom` Symbol which can be used to customize
`util.inspect()` output. Providing `obj[util.inspect.custom]`
works like providing `obj.inspect`, except that the former allows
avoiding name clashes with other `inspect()` methods.

Fixes: nodejs#8071
If a custom inspection function returned `this`, use that value
for further formatting instead of going into infinite recursion.

This is particularly useful when combined with `util.inspect.custom`
because returning `this` from such a method makes it easy to
have an `inspect()` function that is ignored by `util.inspect` without
actually having to provide an alternative for custom inspection.
@addaleax
Copy link
Member Author

Rebased to resolve conflicts with #8189 … new CI: https://ci.nodejs.org/job/node-test-commit/4747/

@targos
Copy link
Member

targos commented Aug 24, 2016

LGTM

@addaleax
Copy link
Member Author

Landed in 59714cb & a60ed89

@addaleax addaleax closed this Aug 25, 2016
@addaleax addaleax deleted the util-inspect-symbol2 branch August 25, 2016 05:22
addaleax added a commit that referenced this pull request Aug 25, 2016
Add a `util.inspect.custom` Symbol which can be used to customize
`util.inspect()` output. Providing `obj[util.inspect.custom]`
works like providing `obj.inspect`, except that the former allows
avoiding name clashes with other `inspect()` methods.

Fixes: #8071
PR-URL: #8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
addaleax added a commit that referenced this pull request Aug 25, 2016
If a custom inspection function returned `this`, use that value
for further formatting instead of going into infinite recursion.

This is particularly useful when combined with `util.inspect.custom`
because returning `this` from such a method makes it easy to
have an `inspect()` function that is ignored by `util.inspect` without
actually having to provide an alternative for custom inspection.

PR-URL: #8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Sep 6, 2016
addaleax added a commit to addaleax/node that referenced this pull request Sep 7, 2016
Add a `util.inspect.custom` Symbol which can be used to customize
`util.inspect()` output. Providing `obj[util.inspect.custom]`
works like providing `obj.inspect`, except that the former allows
avoiding name clashes with other `inspect()` methods.

Fixes: nodejs#8071
PR-URL: nodejs#8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Sep 7, 2016
If a custom inspection function returned `this`, use that value
for further formatting instead of going into infinite recursion.

This is particularly useful when combined with `util.inspect.custom`
because returning `this` from such a method makes it easy to
have an `inspect()` function that is ignored by `util.inspect` without
actually having to provide an alternative for custom inspection.

PR-URL: nodejs#8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
Add a `util.inspect.custom` Symbol which can be used to customize
`util.inspect()` output. Providing `obj[util.inspect.custom]`
works like providing `obj.inspect`, except that the former allows
avoiding name clashes with other `inspect()` methods.

Fixes: nodejs#8071
PR-URL: nodejs#8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

PR-URL: nodejs#8437
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
If a custom inspection function returned `this`, use that value
for further formatting instead of going into infinite recursion.

This is particularly useful when combined with `util.inspect.custom`
because returning `this` from such a method makes it easy to
have an `inspect()` function that is ignored by `util.inspect` without
actually having to provide an alternative for custom inspection.

PR-URL: nodejs#8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

PR-URL: nodejs#8437
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
Add a `util.inspect.custom` Symbol which can be used to customize
`util.inspect()` output. Providing `obj[util.inspect.custom]`
works like providing `obj.inspect`, except that the former allows
avoiding name clashes with other `inspect()` methods.

Fixes: nodejs#8071
PR-URL: nodejs#8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

Refs: nodejs#8437
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
If a custom inspection function returned `this`, use that value
for further formatting instead of going into infinite recursion.

This is particularly useful when combined with `util.inspect.custom`
because returning `this` from such a method makes it easy to
have an `inspect()` function that is ignored by `util.inspect` without
actually having to provide an alternative for custom inspection.

PR-URL: nodejs#8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

Refs: nodejs#8437
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
Add a `util.inspect.custom` Symbol which can be used to customize
`util.inspect()` output. Providing `obj[util.inspect.custom]`
works like providing `obj.inspect`, except that the former allows
avoiding name clashes with other `inspect()` methods.

Fixes: #8071
PR-URL: #8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

Refs: #8437
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
If a custom inspection function returned `this`, use that value
for further formatting instead of going into infinite recursion.

This is particularly useful when combined with `util.inspect.custom`
because returning `this` from such a method makes it easy to
have an `inspect()` function that is ignored by `util.inspect` without
actually having to provide an alternative for custom inspection.

PR-URL: #8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

Refs: #8437
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Sep 9, 2016
Fishrock123 added a commit that referenced this pull request Sep 14, 2016
Notable changes:

* crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
#8304
* events: Made the "max event listeners" memory leak warning more
accessible. (Anna Henningsen) #8298
* promises: Unhandled rejections now emit a process warning after the
first tick. (Benjamin Gruenbaum)
#8223
* repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
#8241
* util: Some functionality has been added to `util.inspect()`:
- Returning `this` from a custom inspect function now works. (Anna
Henningsen) #8174
- Added support for Symbol-based custom inspection methods. (Anna
Henningsen) #8174

Refs: #8428
Refs: #8457
PR-URL: #8466
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 15, 2016
Notable changes:

* crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
nodejs#8304
* events: Made the "max event listeners" memory leak warning more
accessible. (Anna Henningsen) nodejs#8298
* promises: Unhandled rejections now emit a process warning after the
first tick. (Benjamin Gruenbaum)
nodejs#8223
* repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
nodejs#8241
* util: Some functionality has been added to `util.inspect()`:
- Returning `this` from a custom inspect function now works. (Anna
Henningsen) nodejs#8174
- Added support for Symbol-based custom inspection methods. (Anna
Henningsen) nodejs#8174

Refs: nodejs#8428
Refs: nodejs#8457
PR-URL: nodejs#8466
imyller added a commit to imyller/meta-nodejs that referenced this pull request Sep 15, 2016
    Notable changes:

    * crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
    nodejs/node#8304
    * events: Made the "max event listeners" memory leak warning more
    accessible. (Anna Henningsen) nodejs/node#8298
    * promises: Unhandled rejections now emit a process warning after the
    first tick. (Benjamin Gruenbaum)
    nodejs/node#8223
    * repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
    nodejs/node#8241
    * util: Some functionality has been added to `util.inspect()`:
    - Returning `this` from a custom inspect function now works. (Anna
    Henningsen) nodejs/node#8174
    - Added support for Symbol-based custom inspection methods. (Anna
    Henningsen) nodejs/node#8174

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Sep 15, 2016
    Notable changes:

    * crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
    nodejs/node#8304
    * events: Made the "max event listeners" memory leak warning more
    accessible. (Anna Henningsen) nodejs/node#8298
    * promises: Unhandled rejections now emit a process warning after the
    first tick. (Benjamin Gruenbaum)
    nodejs/node#8223
    * repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
    nodejs/node#8241
    * util: Some functionality has been added to `util.inspect()`:
    - Returning `this` from a custom inspect function now works. (Anna
    Henningsen) nodejs/node#8174
    - Added support for Symbol-based custom inspection methods. (Anna
    Henningsen) nodejs/node#8174

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 18, 2016

@addaleax would we want to backport this to v4.x?

edit: if so it needs to include #9289

@addaleax
Copy link
Member Author

addaleax commented Nov 18, 2016

@thealphanerd I’ll open a backport PR and we can talk about that then (but generally I’d be in favour)

EDIT: #9688

addaleax added a commit to addaleax/node that referenced this pull request Nov 18, 2016
Add a `util.inspect.custom` Symbol which can be used to customize
`util.inspect()` output. Providing `obj[util.inspect.custom]`
works like providing `obj.inspect`, except that the former allows
avoiding name clashes with other `inspect()` methods.

Fixes: nodejs#8071
PR-URL: nodejs#8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Nov 18, 2016
If a custom inspection function returned `this`, use that value
for further formatting instead of going into infinite recursion.

This is particularly useful when combined with `util.inspect.custom`
because returning `this` from such a method makes it easy to
have an `inspect()` function that is ignored by `util.inspect` without
actually having to provide an alternative for custom inspection.

PR-URL: nodejs#8174
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@addaleax addaleax mentioned this pull request Nov 18, 2016
4 tasks
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. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants