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: deprecation clarifications #19522

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Mar 21, 2018

Clarify some details on the deprecation policy. Specifically:

  1. Clarify that deprecated code is not subject to the typical support (current or LTS) policies

  2. Clarify that deprecation does not automatically mean removal. Code may be docs-deprecated or runtime deprecated indefinitely.

  3. Clarify that End-of-Life does not necessarily mean removal. What it means is that changes are no longer subject to the semver rules, up to and including removal.

Checklist

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Mar 21, 2018
Documentation-only deprecations may trigger a runtime warning when launched
with [`--pending-deprecation`][] flag (or its alternative,
`NODE_PENDING_DEPRECATION=1` environment variable).
Documentation-only deprecations may trigger an opt-in only runtime warning
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure "opt-in only" is necessary

Copy link
Member

Choose a reason for hiding this comment

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

an opt-in only runtime warning -> a runtime warning

@@ -427,7 +428,9 @@ Node.js uses three Deprecation levels:
deprecated status.

* *End-of-life* refers to APIs that have gone through Runtime Deprecation and
are ready to be removed from Node.js entirely.
are no longer subject to the semantic-versioning and support policies used by
Copy link
Member

Choose a reason for hiding this comment

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

"semantic versioning" without dash

Copy link
Member

Choose a reason for hiding this comment

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

Can we link to these policies here?

__Deprecation__ refers to the identification of Public APIs that should no
longer be used.

Once specific bits of code are marked as being Deprecated, they are no longer
Copy link
Member

@Trott Trott Mar 21, 2018

Choose a reason for hiding this comment

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

specific bits of code are marked as being Deprecated -> APIs are deprecated

longer be used.

Once specific bits of code are marked as being Deprecated, they are no longer
covered by the typical current or Long Term Support policies.
Copy link
Member

@Trott Trott Mar 21, 2018

Choose a reason for hiding this comment

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

Remove typical or current. More importantly, what does this mean? Is this all about semver or is it something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

For instance, a deprecated function in an LTS branch would not fall under the same scrutiny and care as a non-deprecated function in that same branch, meaning we may be more liberal about what changes we allow to be made within those

Copy link
Member

Choose a reason for hiding this comment

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

Can we link to these policies here?

`NODE_PENDING_DEPRECATION=1` environment variable).
Documentation-only deprecations may trigger an opt-in only runtime warning
when Node.js is launched with the [`--pending-deprecation`][] flag (or its
alternative, `NODE_PENDING_DEPRECATION=1` environment variable).
Copy link
Member

@Trott Trott Mar 21, 2018

Choose a reason for hiding this comment

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

(or its alternative, `NODE_PENDING_DEPRECATION=1` environment variable) -> or if the `NODE_PENDING_DEPRECATION` environment variable is set.

Copy link
Member Author

Choose a reason for hiding this comment

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

the value specifically needs to be NODE_PENDING_DEPRECATION=1

@@ -427,7 +428,9 @@ Node.js uses three Deprecation levels:
deprecated status.

* *End-of-life* refers to APIs that have gone through Runtime Deprecation and
are ready to be removed from Node.js entirely.
are no longer subject to the semantic-versioning and support policies used by
the project. Backward-incompatible changes or even complete removal of such
Copy link
Member

Choose a reason for hiding this comment

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

or even -> including

are ready to be removed from Node.js entirely.
are no longer subject to the semantic-versioning and support policies used by
the project. Backward-incompatible changes or even complete removal of such
code may occur at any time.
Copy link
Member

Choose a reason for hiding this comment

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

code -> APIs

Runtime Deprecation cycle. However, there is no requirement that deprecated
code must progress ultimately to *End-of-Life*. It is possible, and at times
preferable, for code that is documentation-only or runtime deprecated to
remain so indefinitely.
Copy link
Member

Choose a reason for hiding this comment

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

It is possible, and at times preferable, for code that is documentation-only or runtime deprecated to remain so indefinitely. -> Documentation-only and runtime deprecations may remain indefinitely.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Fine in concept, just making sure this doesn't land without some of the requested changes...

longer be used.

Once APIs are Deprecated, they are no longer covered by the current or
[Long Term Support][LTS] policies.
Copy link
Member

Choose a reason for hiding this comment

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

That's a link to a repository that doesn't seem to have any policies. Honest, I'm not sure what this sentence means. If it's only about the fact that breaking changes can happen with deprecated APIs, then that's covered later in this doc and much more clearly than here. If it's about something else...what is it?

Copy link
Member

Choose a reason for hiding this comment

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

(I think you can remove this sentence entirely TBQH.)

Documentation-only deprecations may trigger a runtime warning when launched
with [`--pending-deprecation`][] flag (or its alternative,
`NODE_PENDING_DEPRECATION=1` environment variable).
Documentation-only deprecations may trigger an runtime warning when Node.js
Copy link
Member

Choose a reason for hiding this comment

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

an runtime -> a runtime

with [`--pending-deprecation`][] flag (or its alternative,
`NODE_PENDING_DEPRECATION=1` environment variable).
Documentation-only deprecations may trigger an runtime warning when Node.js
is launched with the [`--pending-deprecation`][] flag, or the
Copy link
Member

Choose a reason for hiding this comment

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

launched -> started

remove comma after flag

@@ -427,7 +428,9 @@ Node.js uses three Deprecation levels:
deprecated status.

* *End-of-life* refers to APIs that have gone through Runtime Deprecation and
are ready to be removed from Node.js entirely.
are no longer subject to the semantic versioning and [support policies](LTS)
Copy link
Member

Choose a reason for hiding this comment

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

As above, the link here doesn't provide clarity. Can you be specific about some aspect of deprecated APIs that is different than everything else other than the semantic version stuff? If not, let's remove and support policies.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Thanks for your patience with all the nits!

jasnell added a commit that referenced this pull request Apr 2, 2018
PR-URL: #19522
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Apr 2, 2018

Landed in 7238b92

@jasnell jasnell closed this Apr 2, 2018
@vsemozhetbyt
Copy link
Contributor

It seems this breaks doc linting: https://ci.nodejs.org/job/node-test-linter/17623/console

18:17:16 COLLABORATOR_GUIDE.md
18:17:16   411:1-411:16  warning  Strong should use `*` as a marker   strong-marker        remark-lint
18:17:16         434:82  warning  Line must be at most 80 characters  maximum-line-length  remark-lint
18:17:16 
18:17:16 ⚠ 2 warnings

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 2, 2018

#19762 :)

@Trott
Copy link
Member

Trott commented Apr 2, 2018

It looks like this was landed without running through CI at all.

@Trott
Copy link
Member

Trott commented Apr 2, 2018

(I'm sure this happens a lot, but since I'm noticing it because I'm looking at this: The commit message also does not conform to guidelines. First word should be an imperative verb, such as clarify as in doc: clarify deprecations.)

@vsemozhetbyt
Copy link
Contributor

Mighty spring)

@jasnell
Copy link
Member Author

jasnell commented Apr 3, 2018

Hmm... Something must be out of sync locally on my end then because linting passed locally. Will open a fix PR a bit later this evening

@jasnell
Copy link
Member Author

jasnell commented Apr 3, 2018

Nevermind! Just saw it was already done 😁 thank you kindly!

@Trott
Copy link
Member

Trott commented Apr 3, 2018

Hmm... Something must be out of sync locally on my end then because linting passed locally.

@jasnell I believe there is currently a gotcha where make lint will skip linting the docs if the markdown linter isn't installed. On CI, it always gets installed (I think with make lint-md-build before make lint is run.

targos pushed a commit that referenced this pull request Apr 3, 2018
PR-URL: #19522
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@targos targos mentioned this pull request Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants