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.

Sign up for GitHub

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

Jump to bottom

util: doc deprecate legacy inspect signature #22753

Closed
wants to merge 1 commit into from
Closed

util: doc deprecate legacy inspect signature #22753

wants to merge 1 commit into from

Conversation

refack
Copy link
Contributor

@refack refack commented Sep 7, 2018

Refs: #22751

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. util Issues and PRs related to the built-in util module. labels Sep 7, 2018
@refack refack mentioned this pull request Sep 7, 2018
Closed
4 tasks
@nodejs-github-bot
Copy link
Collaborator

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

targos
targos reviewed Sep 7, 2018
View reviewed changes
doc/api/util.md Outdated
@@ -360,6 +360,8 @@ stream.write('With ES6');
<!-- YAML
added: v0.3.0
changes:
- version ADDME
Copy link
Member

Choose a reason for hiding this comment

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

version: REPLACEME

@refack
Copy link
Contributor Author

refack commented Sep 7, 2018

Since this is deprecating an undocumented feature, I recommend to treat this as semver-patch. As such this needs @nodejs/tsc approval (see some previous discussion in #22751)

@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 7, 2018
@targos
Copy link
Member

targos commented Sep 7, 2018

The collaborator guide says:

Documentation-Only Deprecations may be handled as semver-minor or semver-major changes.

Let's make this semver-minor and it won't need special TSC approval (It will also be highlighted in the release notes, which is a good thing).

targos
targos reviewed Sep 7, 2018
View reviewed changes
doc/api/util.md Outdated
@@ -360,6 +360,8 @@ stream.write('With ES6');
<!-- YAML
added: v0.3.0
changes:
- version: REPLACEME
description: Deprecated legacy signature.
Copy link
Member

Choose a reason for hiding this comment

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

you can already add pr-url: https://github.com/nodejs/node/pull/22753

vsemozhetbyt
vsemozhetbyt reviewed Sep 7, 2018
View reviewed changes
doc/api/util.md Outdated
@@ -424,6 +429,9 @@ changes:
example below. **Default:** `true`.
* Returns: {string} The representation of passed object

Deprecation notice: The legacy function signature that was changed in node 0.9.3
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: node -> Node.js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed it completely (since it could be assumed that Node.js is the object on the sentence)

@refack refack force-pushed the doc-deprecate-inspect-legacy branch from 5c52da1 to 76a64c3 Compare September 7, 2018 19:25
@refack refack force-pushed the doc-deprecate-inspect-legacy branch from 76a64c3 to cfd0067 Compare September 7, 2018 19:26
Trott
Trott reviewed Sep 7, 2018
View reviewed changes
doc/api/util.md
@@ -424,6 +430,9 @@ changes:
example below. **Default:** `true`.
* Returns: {string} The representation of passed object

Deprecation notice: The legacy function signature that was changed in v0.9.3
Copy link
Member

Choose a reason for hiding this comment

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

Nit: remove Deprecation notice:.

Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest simplifying this greatly:

The legacy function signature `util.inspect(object, [showHidden], [depth], [colors])` is deprecated.

Copy link
Member

Choose a reason for hiding this comment

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

One problem with this is that it can be interpreted as saying that util.inspect(object) is deprecated but it is not. Can we indicate that the showHidden, depth, and colors arguments are deprecated instead of pointing to the function signature?

@jasnell
Copy link
Member

jasnell commented Sep 7, 2018

This would need to include a deprecation code assignment in deprecations.md

@refack refack mentioned this pull request Sep 13, 2018
Merged
addaleax
addaleax reviewed Sep 14, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Why do we deprecate this?

doc/api/util.md
@@ -360,6 +360,9 @@ stream.write('With ES6');
<!-- YAML
added: v0.3.0
changes:
- version: REPLACEME
pr-rul: https://github.com/nodejs/node/pull/22753
Copy link
Member

Choose a reason for hiding this comment

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

typo: rul

doc/api/util.md
@@ -424,6 +430,9 @@ changes:
example below. **Default:** `true`.
* Returns: {string} The representation of passed object

Deprecation notice: The legacy function signature that was changed in v0.9.3
`util.inspect(object, [showHidden], [depth], [colors])` has been deprecated.
Copy link
Member

Choose a reason for hiding this comment

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

I’d put this closer to the end of the description… this is probably not relevant to a lot of people (and if it is, we shouldn’t deprecate it)

@trivikr
Copy link
Member

trivikr commented Oct 1, 2018

Ping

@refack refack added the blocked PRs that are blocked by other issues or PRs. label Oct 1, 2018
@refack
Copy link
Contributor Author

refack commented Oct 1, 2018 •
edited
Loading

Why do we deprecate this?

IMHO we should either deprecate or document.
Personally I was pro deprecation, and having a monomorphic signature, but there was pushback.

Ref: #23205

@refack refack closed this Oct 1, 2018
@refack refack deleted the doc-deprecate-inspect-legacy branch October 1, 2018 18:48
@refack refack mentioned this pull request Oct 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@targos targos targos left review comments

@addaleax addaleax addaleax left review comments

@Trott Trott Trott left review comments

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. doc Issues and PRs related to the documentations. semver-minor PRs that contain new features and should be released in the next minor version. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@refack @nodejs-github-bot @targos @jasnell @trivikr @Trott @addaleax @vsemozhetbyt