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

process,doc: add missing deprecation code #37091

Closed
wants to merge 1 commit into from
Closed

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 27, 2021

Refs: #36902

@nodejs-github-bot
Copy link
Collaborator

@Lxxyx 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 aduh95 added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 27, 2021
@aduh95
Copy link
Contributor

aduh95 commented Jan 27, 2021

Fast-track?

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 27, 2021
@aduh95
Copy link
Contributor

aduh95 commented Jan 27, 2021

Landed in 4e833b6

@aduh95 aduh95 closed this Jan 27, 2021
aduh95 pushed a commit that referenced this pull request Jan 27, 2021
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 cjihrig deleted the depr branch January 27, 2021 13:49
@jasnell
Copy link
Member

jasnell commented Jan 27, 2021

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

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 27, 2021

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
Copy link
Contributor

aduh95 commented Jan 27, 2021

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
Copy link
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
Copy link
Member

jasnell commented Jan 27, 2021

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
Copy link
Contributor

aduh95 commented Jan 27, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. 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
Development

Successfully merging this pull request may close these issues.

8 participants