Skip to content

util: inspect - remove very old legacy args - #22751

Closed
refack wants to merge 1 commit into
nodejs:masterfrom
refack:remove-inspect-legacy
Closed

refack wants to merge 1 commit into
nodejs:masterfrom
refack:remove-inspect-legacy

Conversation

@refack

@refack refack commented Sep 7, 2018

Copy link
Copy Markdown
Contributor

Undocumented, and refactored 6 years ago
Refs: 07774e6

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

Undocumented, and refactored 6 years ago
Refs: nodejs@07774e6
@refack refack added wip Issues and PRs that are still a work in progress. util Issues and PRs related to the built-in util module. labels Sep 7, 2018
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Sep 7, 2018

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still used a lot in the wild.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@refack sadly an error occured when I tried to trigger a build :(

@BridgeAR

BridgeAR commented Sep 7, 2018

Copy link
Copy Markdown
Member

I would also like to remove those but before that, we definitely have to add a) a runtime deprecation and b) make sure we open a couple PRs for modules that use the old notation. Using Gzemnid works well for finding those.

I am +1 on changing this to a runtime deprecation.

@mscdex

mscdex commented Sep 7, 2018 •

Copy link
Copy Markdown
Contributor

FWIW it was documented at one point, just a long time ago (v0.8.x seems to the the last branch that had it documented).

@mscdex mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 7, 2018
@refack

refack commented Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

Found this very old legacy compatibility code. I'm seeking feedback on deprecation policy before digging in.
According to the discussion in #14679 and the guides:

node/COLLABORATOR_GUIDE.md

Lines 262 to 263 in e039524

- Any object, property, method, argument, behavior, or event not documented in
the Node.js documentation is internal.

this is "internal" and does not need deprecation.

@refack

refack commented Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

I am +1 on changing this to a runtime deprecation.

it was documented at one point

This is still used a lot in the wild.

In that case could we jump straight to runtime deprecation?

@mscdex

mscdex commented Sep 7, 2018 •

Copy link
Copy Markdown
Contributor

I disagree on calling it internal as it was a publicly documented signature, however at the time the documentation was changed, there was no mechanism in place to deprecate a specific function signature. If we are going to officially deprecate it, I would rather start with a documentation deprecation as I'm sure there are people still using this signature (myself included as it it's easier/faster to type and more succinct). The other choice is to re-document it.

@BridgeAR

BridgeAR commented Sep 7, 2018

Copy link
Copy Markdown
Member

@mscdex IMHO a documentation deprecation would be the opposite of what we would want to achieve: this is current not documented at all. Therefore adding a doc only deprecation would actually mean we have to add that part to the docs or no one would ever stumble upon it. Thus, I am definitely only +1 for a runtime deprecation. Otherwise we can just keep it as is.

@mscdex

mscdex commented Sep 7, 2018

Copy link
Copy Markdown
Contributor

@BridgeAR We've document-deprecated "private" properties/methods before and those were never documented. I don't see why document-deprecating something that was once publicly documented to be a problem.

@refack refack added blocked PRs that are blocked by other issues or PRs. and removed blocked PRs that are blocked by other issues or PRs. labels Sep 7, 2018
@refack

refack commented Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

Found the last reference it's in:
https://nodejs.org/download/release/v0.9.2/docs/api/util.html
not in
https://nodejs.org/download/release/v0.9.3/docs/api/util.html

a documentation deprecation would be the opposite of what we would want to achieve

We could add a line saying:

Deprecated: The legacy function signature that was changed in node 0.9.3 `util.inspect(object, [showHidden], [depth], [colors])`

I'd recommend we add this now, and a runtime in 11. That should get TSC approval.

@refack refack mentioned this pull request Sep 7, 2018
3 tasks done
@refack

refack commented Sep 7, 2018

Copy link
Copy Markdown
Contributor Author

See #22753

refack added a commit to refack/node that referenced this pull request Sep 7, 2018
Comment thread lib/util.js
for (var i = 0; i < optKeys.length; i++) {
ctx[optKeys[i]] = opts[optKeys[i]];
if (options !== undefined) {
if (typeof options !== 'object') {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"options" can be null also

@refack refack added blocked PRs that are blocked by other issues or PRs. and removed wip Issues and PRs that are still a work in progress. labels Oct 1, 2018
@refack

refack commented Oct 1, 2018 •

Copy link
Copy Markdown
Contributor Author

Seems there is consesouns to document instead.

Refs: #23205

@refack refack closed this Oct 1, 2018
@refack
refack deleted the remove-inspect-legacy branch October 1, 2018 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blocked PRs that are blocked by other issues or PRs. semver-major PRs that contain breaking changes and should be released in the next major 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.

5 participants