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

repl: update deprecation codes #33430

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 16, 2020

Refs: #33294

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label May 16, 2020
@targos
Copy link
Member

targos commented May 16, 2020

I don't remember where our documentation says that we should use placeholders for deprecations, but can we change it? We forget to update it way too often and it seems possible to me to have a lint rule to validate the numbers in deprecations.md (no duplicate, sequential).

@aduh95 aduh95 mentioned this pull request May 16, 2020
4 tasks
@BridgeAR
Copy link
Member

Collaborators please 👍 to fast track this.

@BridgeAR
Copy link
Member

@targos I agree that we should change that. To verify that we have unique codes, we should just add a test that verifies that the codes are unique in our markdown file.

@addaleax addaleax added the fast-track PRs that do not need to wait for 48 hours to land. label May 16, 2020
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 18, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 18, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/31374/ ✅ (all green besides a known CI issue that is unrelated)

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 18, 2020
Refs: nodejs#33294

PR-URL: nodejs#33430
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BridgeAR
Copy link
Member

Landed in a12a2d8 🎉

@codebytere
Copy link
Member

Adding the dont-land-on-v14.x label as i did this when i released v14.3.0.

@jasnell
Copy link
Member

jasnell commented May 29, 2020

I agree that we should change that. To verify that we have unique codes, we should just add a test that verifies that the codes are unique in our markdown file.

Sidenote: this was actually the intention originally but it never manifested for various reasons. The main difficulty here with deprecation codes is that we do not know when they are actually going to land in a release. What I would prefer is that we just assign the deprecation numbers in the release just like we handle the replaceme tags for versions. We would have to alter the testing strategy somewhat but that's fairly simple.

@BridgeAR
Copy link
Member

@jasnell that would not work for backports. It should be possible to just lint for unique deprecation codes. That way we'll immediately know if a PR needs to update before landing.

@jasnell
Copy link
Member

jasnell commented May 29, 2020

Well, I'd argue that we likely shouldn't be backporting deprecations but since we stopped requiring doc only deprecations to be major I guess that ship has sailed. Another approach would be to move away from numbered dep codes entirely and use simple labels. DEP_THING style. Lint rules can enforce a style and uniqueness and we can skip the need to apply a sequential numbering at all.

@aduh95 aduh95 deleted the update-deprecation-codes branch May 29, 2020 09:03
@aduh95 aduh95 mentioned this pull request Jul 14, 2020
4 tasks
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. fast-track PRs that do not need to wait for 48 hours to land. repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants