Skip to content

doc: order deprecation reasons#27960

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
Closed
trivikr wants to merge 6 commits into from
Closed

doc: order deprecation reasons #27960

trivikr wants to merge 6 commits into from

Conversation

@trivikr
Copy link
Member

@trivikrtrivikr commented May 29, 2019

Checklist

ZYSzys
ZYSzys reviewed May 30, 2019
View reviewed changes
@ZYSzysZYSzys added doc Issues and PRs related to the documentations. deprecations Issues and PRs related to deprecations. labels May 30, 2019
Trott
Trott reviewed May 30, 2019
View reviewed changes
@Trott
Copy link
Member

Trott commented May 30, 2019

Potentially-unpopular opinion: All of this text can be deleted. It lists the reasons any platform might deprecate an API. Nothing in there is specific or peculiar to Node.js. And it's not clear that it's a comprehensive list. We don't need to list why we deprecate when those reasons are the same as every other platform. We can go right into the different kinds of deprecations.

@Trott
Copy link
Member

Trott commented May 30, 2019

One potential problem with this change: The current wording (mostly) makes it clear that any one of the three possibilities is enough to deprecate. This new format might lead one to think that all three things must be true.

(Another argument for just deleting it!)

@Trott
Copy link
Member

Trott commented May 30, 2019•
edited
Loading

Lastly: All my comments here are non-blocking. There's always going to be room for improvement. Docs! Amirite?

trivikr
trivikr commented May 30, 2019
View reviewed changes
Trott
Trott reviewed Jun 2, 2019
View reviewed changes
Trott
Trott reviewed Jun 2, 2019
View reviewed changes
Trott
Trott approved these changes Jun 2, 2019
View reviewed changes
Copy link
Member

@TrottTrott left a comment

Choose a reason for hiding this comment

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

LGTM with punctuation nits.

@Trott
Copy link
Member

Trott commented Jun 2, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3647/

@Trott
Copy link
Member

Trott commented Jun 2, 2019•
edited
Loading

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3648/

@trivikrtrivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 3, 2019
@trivikrtrivikr self-assigned this Jun 3, 2019
@trivikrtrivikrforce-pushed the deprecation-reasons branch from 00abf78 to dd75200CompareJune 3, 2019 19:30
@trivikr
Copy link
MemberAuthor

trivikr commented Jun 3, 2019

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3649/

@trivikr
Copy link
MemberAuthor

trivikr commented Jun 3, 2019

ping @Trott@ZYSzys for review post rebase

Trott
Trott reviewed Jun 6, 2019
View reviewed changes
Trott
Trott approved these changes Jun 6, 2019
View reviewed changes
Copy link
Member

@TrottTrott left a comment

Choose a reason for hiding this comment

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

LGTM with or without my remaining suggestion applied

@trivikr
Copy link
MemberAuthor

trivikr commented Jun 6, 2019

Landed in 5c61c5d

@trivikrtrivikr closed this Jun 6, 2019
@trivikrtrivikr deleted the deprecation-reasons branch June 6, 2019 05:51
trivikr added a commit to trivikr/node that referenced this pull request Jun 6, 2019
@trivikr
doc: order deprecation reasons
5c61c5d
PR-URL: nodejs#27960 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
@trivikr@BridgeAR
doc: order deprecation reasons
9dd4813
PR-URL: #27960 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Yongsheng Zhang <[email protected]>
@BridgeARBridgeAR mentioned this pull request Jun 17, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes

@ZYSzysZYSzysZYSzys approved these changes

Assignees

@trivikrtrivikr

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.deprecationsIssues and PRs related to deprecations.docIssues and PRs related to the documentations.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@trivikr@Trott@ZYSzys