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: inspect - remove very old legacy args #22751

Closed
wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented Sep 7, 2018

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
Copy link
Member

@BridgeAR BridgeAR left a comment

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
Collaborator

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

@BridgeAR
Copy link
Member

BridgeAR commented Sep 7, 2018

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
Copy link
Contributor

mscdex commented Sep 7, 2018

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

refack commented Sep 7, 2018

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

refack commented Sep 7, 2018

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
Copy link
Contributor

mscdex commented Sep 7, 2018

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
Copy link
Member

BridgeAR commented Sep 7, 2018

@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
Copy link
Contributor

mscdex commented Sep 7, 2018

@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
Copy link
Contributor Author

refack commented Sep 7, 2018

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

refack commented Sep 7, 2018

See #22753

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

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

refack commented Oct 1, 2018

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