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

doc: lint deprecation codes #33433

Closed
wants to merge 2 commits into from
Closed

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 16, 2020

Add a rule to make sure deprecation codes are in order. This is to discourage the use of placeholders by contributors when creating new deprecations, and to relieve authors the responsibility to swap them with the appropriate code on landing.

Suggested by @targos in #33430 (comment).

Blocked by #33430.

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 doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels May 16, 2020
doc/api/deprecations.md Outdated Show resolved Hide resolved
@richardlau
Copy link
Member

Is it possible to implement this as a lint rule as opposed to a new tool and test?

@aduh95
Copy link
Contributor Author

aduh95 commented May 16, 2020

Is it possible to implement this as a lint rule as opposed to a new tool and test?

@richardlau Do you have an example I can follow on how to implement that? I'm not familiar with markdown linters (yet 😅).

@richardlau
Copy link
Member

richardlau commented May 16, 2020

https://github.com/nodejs/node/blob/master/tools/eslint-rules/documented-errors.js, for example.

Ah wait that is a js lint rule. I think the custom markdown lint rules are external to this repo. Cc @Trott

@BridgeAR
Copy link
Member

@richardlau we could lint that all deprecation codes adhere to a specific format in lib.

Add a rule to make sure deprecation codes are in order.
@aduh95 aduh95 marked this pull request as ready for review May 18, 2020 08:23
@aduh95
Copy link
Contributor Author

aduh95 commented May 29, 2020

@richardlau we could lint that all deprecation codes adhere to a specific format in lib.

That would cover runtime deprecation only right?

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Jul 4, 2020

Ah wait that is a js lint rule. I think the custom markdown lint rules are external to this repo. Cc @Trott

They are in https://github.com/nodejs/remark-preset-lint-node.

@aduh95
Copy link
Contributor Author

aduh95 commented Oct 16, 2020

Do we still want to do that? Every now and then a new PR appears to replace DEPXXX placeholders that made their way to master, but on the other hand the tooling is supposed to take care of it now…

I personally don't get why we use a placeholder, because if a new deprecation lands before the one on the PR, the author still has to fix the git conflict, so the placeholder doesn't help there. If someone thinks it's still a valid idea, I'm willing to work on it, otherwise let's close this.

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Oct 16, 2020
@github-actions
Copy link
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@Trott
Copy link
Member

Trott commented Oct 17, 2020

I personally don't get why we use a placeholder, because if a new deprecation lands before the one on the PR, the author still has to fix the git conflict, so the placeholder doesn't help there. If someone thinks it's still a valid idea, I'm willing to work on it, otherwise let's close this.

In theory at least, one thing the placeholder gets us is prevention of duplicate DEP codes landing and nobody noticing.

@aduh95 aduh95 closed this Nov 14, 2020
@aduh95 aduh95 deleted the lint-deprecation-codes branch November 14, 2020 11:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stalled Issues and PRs that are stalled. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants