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

doc: order deprecation reasons #27960

Closed
wants to merge 6 commits into from
Closed

doc: order deprecation reasons #27960

wants to merge 6 commits into from

Conversation

trivikr
Copy link
Member

@trivikr trivikr commented May 29, 2019

Checklist

ZYSzys
ZYSzys reviewed May 30, 2019
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
@ZYSzys ZYSzys 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
doc/api/deprecations.md Outdated Show resolved Hide resolved
@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
doc/api/deprecations.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Jun 2, 2019
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
Trott
Trott reviewed Jun 2, 2019
View reviewed changes
doc/api/deprecations.md Outdated Show resolved Hide resolved
Trott
Trott approved these changes Jun 2, 2019
View reviewed changes
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.

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/

@trivikr trivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 3, 2019
@trivikr trivikr self-assigned this Jun 3, 2019
trivikr and others added 6 commits June 3, 2019 12:30
@trivikr
doc: order deprecation reasons
05f668b
@trivikr @Trott
doc: suggestion from Trott
07c4177 
Co-Authored-By: Rich Trott <rtrott@gmail.com>
@trivikr
doc: uppercase the first letter in list
6c231f2
@trivikr @Trott
doc: add any of the following
88c7acf 
Co-Authored-By: Rich Trott <rtrott@gmail.com>
@trivikr @Trott
Update doc/api/deprecations.md
0e5235b 
Co-Authored-By: Rich Trott <rtrott@gmail.com>
@trivikr @Trott
Update doc/api/deprecations.md
dd75200 
Co-Authored-By: Rich Trott <rtrott@gmail.com>
@trivikr
Copy link
Member Author

trivikr commented Jun 3, 2019

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

@trivikr
Copy link
Member Author

trivikr commented Jun 3, 2019

ping @Trott @ZYSzys for review post rebase

Trott
Trott approved these changes Jun 6, 2019
View reviewed changes
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.

LGTM with or without my remaining suggestion applied

@trivikr
Copy link
Member Author

trivikr commented Jun 6, 2019

Landed in 5c61c5d

@trivikr trivikr closed this Jun 6, 2019
@trivikr trivikr 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 <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
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 <rtrott@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZYSzys ZYSzys ZYSzys approved these changes

@Trott Trott Trott approved these changes

Assignees

@trivikr trivikr

Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. deprecations Issues and PRs related to deprecations. doc Issues 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