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 to disable only custom inspect "named" method #16266

Closed
wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack 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

@refack refack 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

jasnell commented Oct 18, 2017

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
Contributor Author

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

jasnell commented Oct 23, 2017

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

@jasnell jasnell requested a review from a team October 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

mscdex commented Oct 23, 2017

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

jasnell commented Nov 22, 2017

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

BridgeAR commented Dec 6, 2017

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

@BridgeAR BridgeAR closed this Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

5 participants