-
Notifications
You must be signed in to change notification settings - Fork 29k
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
doc: clarify util.inspect usage intent #17375
Conversation
doc/api/util.md
Outdated
@@ -348,8 +348,9 @@ changes: | |||
line. Defaults to 60 for legacy compatibility. | |||
|
|||
The `util.inspect()` method returns a string representation of `object` that is | |||
primarily useful for debugging. Additional `options` may be passed that alter | |||
certain aspects of the formatted string. | |||
intended for debugging. **The output of `util.inspect` may change at any time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No bold text please.
Is the added text true? Might we not consider a significant change to |
(Change looks good to me if it's accurate, of course. Would definitely prefer no bold text. We overuse arbitrary typographical emphasis in our docs.) |
I know I mentioned that we should document it in the other issue if that is the case, but I'm pretty against this. I have code that relies on the output of util.inspect and I know that others in the ecosystem do as well. |
i feel like relying on the output of something that's designed to present human readable information is kinda irresponsible, perhaps we can land this in 10, and then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think this warning is a good idea.
In particular, this does not mean that we can’t consider breaking changes to util.inspect
semver-major in the future, just that relying on its output is a bad practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better if we called this out in a separate note or warning section, so that the importance of this is highlighted?
TL;DR: No. :-D I'd rather not give it undue weight. We've survived this long without a note at all. We can always emphasize it if it becomes apparent that it's critical (which, honestly, won't happen). We have a tendency to want all new notes to be set off in a separate bolded or italicized or otherwise-highlighted note. By emphasizing everything, we emphasize nothing. Honestly, this information is good to have, but not more important than lots of other things in the doc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@devsnek Could you rebase this? |
@addaleax done 👍 |
I would like to land this in the next few days unless there are objections. |
certain aspects of the formatted string. | ||
intended for debugging. The output of `util.inspect` may change at any time | ||
and should not be depended upon programmatically. Additional `options` may be | ||
passed that alter certain aspects of the formatted string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow this sounds a bit harsh to me. Would you be fine to change it to something like
The `util.inspect()` method returns a string representation of `object` that is
primarily intended for debugging. Additional `options` may be passed that alter
certain aspects of the formatted string.
Note that the output of `util.inspect` may undergo minor changes from time to
time without a semver-major step. Programmatically relying on the output is
therefore not recommended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i kinda want it to be harsh, there's a reason stuff like internalBinding
has to be a thing now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With your comment in place as is I would argue that landing #17576 as new default without putting it behind a option could be done as a semver-patch. And I do not see that at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With your comment in place as is I would argue that landing #17576 as new default without putting it behind a option could be done as a semver-patch. And I do not see that at all.
I'd rather we be harsher in the docs , but more conservative when actually landing possibly breaking PRs, so +1 on keeping this wording, but making #17576 major
.
Realistically we know that if enough people depend on the output of util.inspect
, no matter how internal, we won't be able to change it. But if this warning helps dissuade people from relying on that output, then that seems like a good thing.
Also if this PR makes #17576 a patch, that would make this PR semver-major 😁 .
PR-URL: nodejs#17375 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Landed in 3b98038 |
commit from #16956 moved to separate pr by request of @addaleax
Checklist
Affected core subsystem(s)