-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: changed util.inspect signature #23216
Closed
Closed
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
605a893
doc : changed util.inspect signature
siddhant1 127ef47
doc: changed util.inspect signature
siddhant1 4bc07c2
Update util.md
siddhant1 cf6409f
doc: changed util.inspect signature
siddhant1 3c391c1
doc: changed util.inspect signature
siddhant1 57cc11a
doc: changed util.inspect signature
siddhant1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This does not seem to be completely correct. You want to add the legacy signature which is:
The options signature is not compatible with it since it's not possible to combine the legacy one and the new one. So a separate entry would be required in this case. However, I am not fond of actually documenting it. It uses boolean arguments and those are difficult to grasp without actively looking into the documentation.
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.
Somebody rose an issue on this one , are you sure you don't want it documented
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.
If the signature is "difficult to grasp without actively looking into the documentation", that would seem to argue for documenting it. I'd certainly be in favor of applying a doc-only deprecation to that signature (if it's not already deprecated) though.
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.
(Specifically: If someone comes across
util.inspect()
being used this way, they should be able to find the signature in the documentation. The deprecation would be to discourage its further use this way. But people should still be able to use the docs to help them figure out code that already exists.)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.
Can you tell me the correct signature so I can make another pr on this one?
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 believe there's needs to be two separate signatures, one for when there is an
options
object and one for when there are the other arguments.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.
So I'm guessing the answer to @siddhant1's question is:
AND
Is that right @Trott, @refack?