FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

process,doc: add missing deprecation code by cjihrig · Pull Request #37091 · nodejs/node · GitHub

/ node Public

process,doc: add missing deprecation code - #37091

Closed
cjihrig wants to merge 1 commit into
nodejs:masterfrom
cjihrig:depr
ConversationCommits (1)ChecksFiles changed
Closed

process,doc: add missing deprecation code#37091
cjihrig wants to merge 1 commit into
nodejs:masterfrom
cjihrig:depr

Conversation

cjihrig commented Jan 27, 2021

Copy link
Copy Markdown
Contributor

Refs: #36902

nodejs-github-bot commented Jan 27, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35768/

Lxxyx added doc Issues and PRs related to the documentations. deprecations Issues and PRs related to deprecations. process Issues and PRs related to the process subsystem. labels Jan 27, 2021
aduh95 added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 27, 2021

aduh95 commented Jan 27, 2021

Copy link
Copy Markdown
Contributor

Fast-track?

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 27, 2021

aduh95 commented Jan 27, 2021

Copy link
Copy Markdown
Contributor

Landed in 4e833b6

aduh95 closed this Jan 27, 2021
aduh95 pushed a commit that referenced this pull request Jan 27, 2021
process,doc: add missing deprecation code
4e833b6 
Refs: #36902

PR-URL: #37091
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
cjihrig deleted the depr branch January 27, 2021 13:49

jasnell commented Jan 27, 2021

Copy link
Copy Markdown
Member

Arg... thanks. I keep forgetting that the git node tooling doesn't catch these. It does make me wonder if it could tho...

cjihrig commented Jan 27, 2021

Copy link
Copy Markdown
Contributor Author

It does make me wonder if it could tho...

node/Makefile

Lines 933 to 937 in c9992a0

@if [ "$(DISTTYPE)" = "release" ] && \
`grep -q DEP...X doc/api/deprecations.md`; then \
echo 'Please update DEP...X in doc/api/deprecations.md (See doc/guides/releases.md)' ; \
exit 1 ; \
fi

I imagine it just needs something like this.

aduh95 commented Jan 27, 2021

Copy link
Copy Markdown
Contributor

Arg... thanks. I keep forgetting that the git node tooling doesn't catch these. It does make me wonder if it could tho...

Or maybe we could stop using DEPXXXX and put the actual deprecation code in the original PR… I tried to do that in #33433 but that didn't land.

richardlau commented Jan 27, 2021

Copy link
Copy Markdown
Member

Arg... thanks. I keep forgetting that the git node tooling doesn't catch these. It does make me wonder if it could tho...

nodejs/node-core-utils#420 was supposed to detect these.

jasnell commented Jan 27, 2021

Copy link
Copy Markdown
Member

Or maybe we could stop using DEPXXXX and put the actual deprecation code in the original PR… I tried to do that in #33433 but that didn't land.

Key challenge with that is maintaining the order on landing (e.g. if I open a semver-major deprecation PR today that takes three months to land, but three other semver-minor doc only deprecations happen in the meantime... which has happened before).

The one thing we could do is move away from numbered deprecation codes at all and move to a non-numeric code, e.g. DEPWHATEVER but it can be difficult to come up with good names so we'd likely just be trading one problem for another.

aduh95 commented Jan 27, 2021

Copy link
Copy Markdown
Contributor

Key challenge with that is maintaining the order on landing (e.g. if I open a semver-major deprecation PR today that takes three months to land, but three other semver-minor doc only deprecations happen in the meantime... which has happened before).

My experience with PR for deprecation that stay open for a long time is that deprecations that are added in the mean time always create a git conflict anyway. So the PR author has to rebase to fix the git conflict, they may as well update the deprecation code. If you add to that a lint rule which would pick up duplicate deprecation codes, I think we would have a way better system that we have now.

BethGriggs mentioned this pull request Apr 12, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Lxxyx Lxxyx approved these changes
aduh95 aduh95 approved these changes
RaisinTen RaisinTen approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. process Issues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL