util: change inspect's default depth from 2 to 4

[silverwind]

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

When dealing with somewhat deep objects, the default depth of 2 is often a hindrance when using the console to log said objects. This changes the default depth to a more reasonable 4.

[nodejs-github-bot]

@silverwind build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/999/pipeline

[silverwind]

Not sure what semver to set here. If inspect output is considered breaking, we may have to semver-major it.

[addaleax]

I would consider this (like the alternative PR at #22846) to be semver-major, yes.

[mscdex]

Didn't we just change this to 2 from 4 not that long ago?

EDIT: Apparently not

[jdalton]

@silverwind

I think #23062 is a better alternative to this PR. Sorry for not noticing this PR earlier.

I'm not a fan of the approach taken here; opening in a dup PR a week+ later, then going to the original PR (#22846) and red X'ing it. I think it'd be better to instead work with #22846 to get a desirable outcome.

[addaleax]

@jdalton I believe @silverwind that this was not intentional.

Is your “changes requested” marker based solely on the way in this PR was opened?

[jdalton]

@addaleax I'd rather there be some discussion before red-x'ing an issue. To come to #22846, fired off a couple comments, and then red-X it in favor of their own, without giving anyone time to respond or have a discussion doesn't seem like a good practice to encourage.

As for the depth change I'm cautiously optimistic. I think a depth of 20, as in #22846, is a little aggressive, and I think 4 isn't much better than 2. I've not been irked by the default limits so far and haven't had a strong urge to change them.

[mscdex]

@jdalton I don't think closing this PR for that reason was the correct decision here. If there were significant -1's, then perhaps that would be grounds for closing the PR, but not because of a non-technical disagreement.

[jdalton]

Oops. The classic comment vs. comment-close mixup. Closing not intended.

[addaleax]

@jdalton Like I said, I think this was not intentional on @silverwind’s part and I would probably not have red-X-ed #22846 since it seemed like I am the only one being opposed to the higher limit before this PR showed up independently.

So, just to clarify, your “requested changes” marker is based only so this PR doesn’t move forward because there already is another one? I don’t think that’s helpful given that @BridgeAR has indicated no desire to adjust #22846 even though there were previous comments requesting a less drastic change.

[jdalton]

@addaleax

I appreciate your input. I don't think there has been enough discussion on #22846. I believe @BridgeAR has just gotten to explaining the reason for choosing a depth of 20 and is now waiting for a response for input on why a depth of 20 isn't a good idea (please follow up on #22846).

I have objections for how this was handled in process as well as implementation (the depth of 4). I want to be very cautious about this kind of behavior (opening an opposing PR and red-x-ing the other without discussion). To me that's a very bully-like behavior and should have some call-out of caution.

If we're blocked on a depth 20 and folks agree that 4 is better than 2 then we can totally move forward, but for now I think #22846 is a better PR (ignoring the specific depth value) because it's covering more places depth is hardcoded (more thorough).

[addaleax]

To me that's a very bully-like behavior and should have some call-out of caution.

I agree that it’s something we should avoid, if we can. I don’t know if it’s feasible, but Google’s (?) PolyGerrit shows CLs that would lead to merge conflicts with the one currently being viewed. Maybe we could get something like that into the bot…

but for now I think #22846 is a better PR (ignoring the specific depth value) because it's covering more places depth is hardcoded (more thorough).

That is a good point, and it’s something I’d like to see independently of which PR we go with. I’ll ask about splitting the commit for %o out from #22846 over there.

[BridgeAR]

@addaleax

I don’t think that’s helpful given that @BridgeAR has indicated no desire to adjust #22846 even though there were previous comments requesting a less drastic change.

I am not aware that I said I would not do something like that. So far there was a comment that expressed a concern and I replied to it. I actually gave multiple extra days to express further concerns.

[silverwind]

I want to be very cautious about this kind of behavior (opening an opposing PR and red-x-ing the other without discussion). To me that's a very bully-like behavior and should have some call-out of caution.

Yes, sorry about that. I just put it there to request further discussion because a PR with many checkmarks might be tempting to merge for new collaborators. Let's not split up the discussion and continue in #22846. I'm happy either way as long as the too low default of 2 is abolished in the end 😉 .

[silverwind]

Closing in favor of #22846