Skip to content

Conversation

@refack
Copy link
Contributor

@refackrefack commented Oct 17, 2017

Refs: #15549
Refs: #15631

This add a switch that allows disabling just the "named" custom inspect method.
Except for the immediate benefit, this also allows future deprecation by defaulting to false and warning only on changing the default. Or allowing to turn this off so that deprecation warning will not be emitted.

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

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

util

@refackrefack added 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. labels Oct 17, 2017
@jasnell
Copy link
Member

The code changes LGTM, and I'm fine with the idea, I'm just not entirely sure if this is the right approach. I could be easily swayed, however. I'm curious how other @nodejs/collaborators feel about it.

@BridgeAR
Copy link
Member

Hm, the code change itself looks good to me but adding a specific option for such a edge case feels a bit much to me.

@refack
Copy link
ContributorAuthor

refack commented Oct 22, 2017

As I see it it's the way to give control to the user of util.inspect since in most cases the "offending" code will probably come from a 3rd party library.
#16393 which is the more straightforward approach, faces that challenge and some performance challenges.

This approach while adding an API, seems like it's simpler codewise. Also since the runtime deprecation will not make it to 9.0.0, this hack would probably be with us for at least one more year.

@jasnell
Copy link
Member

I definitely see the reasoning behind this but I'm still a bit unsure. Hey @nodejs/tsc... any thoughts?

@jasnelljasnell requested a review from a teamOctober 23, 2017 16:47
@addaleax
Copy link
Member

Btw, practically speaking you can already choose which behaviour you want by setting Object.prototype[util.inspect.custom]

@mscdex
Copy link
Contributor

I also think a command line flag just for this is a bit much.

@BridgeAR
Copy link
Member

@refack was there ever a request to have the possibility to do exactly this? Because I am not aware of that. So it would be adding a option that is likely not required and in that case I would like to go ahead and close this PR.

@jasnell
Copy link
Member

I think I'm -1 on this particular change for precisely the reason @addaleax mentions.

@addaleax
Copy link
Member

@refack If you do want to continue pursuing this, this PR would need a rebase

@BridgeAR
Copy link
Member

Closing due to no response and because four people are not in favor of this.

@BridgeARBridgeAR closed this Dec 6, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver-minorPRs that contain new features and should be released in the next minor version.utilIssues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@refack@jasnell@BridgeAR@addaleax@mscdex