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

doc: clearer doc-only deprecations #20381

Closed
wants to merge 2 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions COLLABORATOR_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -416,14 +416,15 @@ longer be used.

Node.js uses three Deprecation levels:

* *Documentation-Only Deprecation* refers to elements of the Public API that are
being staged for deprecation in a future Node.js major release. An explicit
notice indicating the deprecated status is added to the API documentation
but no functional changes are implemented in the code. There will be no
runtime deprecation warnings emitted for such deprecations by default.
Documentation-only deprecations may trigger a runtime warning when Node.js
is started with the [`--pending-deprecation`][] flag or the
`NODE_PENDING_DEPRECATION=1` environment variable is set.
* *Documentation-Only Deprecation* refers to elements of the Public API that
should be avoided by developers and that might be staged for deprecation in a
Copy link
Member

@Trott Trott Apr 28, 2018

Choose a reason for hiding this comment

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

The word "deprecation" does not mean removal. It means discouragement of use. So "staged for deprecation" makes no sense in this context. A documentation deprecation is a deprecation. I think you might mean "staged for removal"? Although I would just go with "removed":

should be avoided by developers and that might be removed in a

Copy link
Member

Choose a reason for hiding this comment

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

I see that "staged for deprecation" was existing text, not new text introduced here. I'd still encourage removing it as it is ambiguous at best. "Removed" is clear. Or if you mean a runtime warning, then say that. Either way, more precise and explicit is better.

Copy link
Member

Choose a reason for hiding this comment

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

(Above comments are suggestions, but not blocking objections.)

future Node.js major release. An explicit notice indicating the deprecation
status is added to the API documentation but no functional changes are
implemented in the code. There will be no runtime deprecation warnings emitted
for such deprecations at runtime by default. Documentation-only deprecations
Copy link
Member

Choose a reason for hiding this comment

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

Please remove either the first or the last use of "runtime" in "runtime deprecation warnings emitted for such deprecations at runtime".

may trigger a runtime warning when Node.js is started with the
[`--pending-deprecation`][] flag or the `NODE_PENDING_DEPRECATION=1`
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 28, 2018

Choose a reason for hiding this comment

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

Nit: maybe it is worth to also linkify [`NODE_PENDING_DEPRECATION=1`][] and add the bottom reference:

[`NODE_PENDING_DEPRECATION=1`]: doc/api/cli.md#node_pending_deprecation1

environment variable is set.

* *Runtime Deprecation* refers to the use of process warnings emitted at
runtime the first time that a deprecated API is used. A command-line
Expand Down