Skip to content

Conversation

@cjihrig
Copy link
Contributor

Refs: #36902

@nodejs-github-bot
Copy link
Collaborator

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

Fast-track?

@aduh95aduh95 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

Landed in 4e833b6

@aduh95aduh95 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 <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
@cjihrigcjihrig deleted the depr branch January 27, 2021 13:49
@jasnell
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...

@cjihrig
Copy link
ContributorAuthor

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

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

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

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

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.fast-trackPRs that do not need to wait for 48 hours to land.processIssues and PRs related to the process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@cjihrig@nodejs-github-bot@aduh95@jasnell@richardlau@Lxxyx@RaisinTen@targos