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
feat!: Remove valid-jsdoc and require-jsdoc #17694
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
Yep, just fixed it. |
conf/rule-type-list.json
Outdated
@@ -23,6 +23,7 @@ | |||
{ "removed": "space-in-brackets", "replacedBy": ["object-curly-spacing", "array-bracket-spacing"] }, | |||
{ "removed": "space-return-throw-case", "replacedBy": ["keyword-spacing"] }, | |||
{ "removed": "space-unary-word-ops", "replacedBy": ["space-unary-ops"] }, | |||
{ "removed": "spaced-line-comment", "replacedBy": ["spaced-comment"] } | |||
{ "removed": "spaced-line-comment", "replacedBy": ["spaced-comment"] }, | |||
{ "removed": "valid-jsdoc", "replacedBy": [] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also add require-jsdoc
to this list.
docs/src/rules/require-jsdoc.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not removing docs for removed rules. We're just updating it with the info that the rule has been removed.
For example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Pretty sure we had a stronger visual treatment than that, though. I'll check on that too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems that on the old site that (removed)
was converted to an X.
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
docs/src/use/rule-deprecation.md
Outdated
@@ -7,10 +7,12 @@ Balancing the trade-offs of improving a tool and the frustration these changes c | |||
|
|||
The ESLint team is committed to making upgrading as easy and painless as possible. To that end, the team has agreed upon the following set of guidelines for deprecating rules in the future. The goal of these guidelines is to allow for improvements and changes to be made without breaking existing configurations. | |||
|
|||
* Rules will never be removed from ESLint. | |||
* Rules will never be removed from ESLint unless one of the following is true: | |||
* The rule has been replaced by another core rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* The rule has been replaced by another core rule | |
* The rule has been replaced by another core rule. |
tests/lib/cli-engine/cli-engine.js
Outdated
@@ -1774,7 +1772,7 @@ describe("CLIEngine", () => { | |||
engine = new CLIEngine({ | |||
cwd: originalDir, | |||
useEslintrc: false, | |||
rules: { indent: 1, "valid-jsdoc": 0, "require-jsdoc": 0 } | |||
rules: { indent: 1, "callback-return": 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is causing merge conflicts. The fixed line should be rules: { eqeqeq: 1, "callback-return": 0 }
(indent
was replaced here with eqeqeq
in #17696).
tests/lib/eslint/eslint.js
Outdated
@@ -1762,7 +1760,7 @@ describe("ESLint", () => { | |||
cwd: originalDir, | |||
useEslintrc: false, | |||
overrideConfig: { | |||
rules: { indent: 1, "valid-jsdoc": 0, "require-jsdoc": 0 } | |||
rules: { indent: 1, "callback-return": 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as #17694 (comment)
tests/lib/eslint/flat-eslint.js
Outdated
@@ -1956,7 +1954,7 @@ describe("FlatESLint", () => { | |||
cwd: originalDir, | |||
overrideConfigFile: true, | |||
overrideConfig: { | |||
rules: { indent: 1, "valid-jsdoc": 0, "require-jsdoc": 0 } | |||
rules: { indent: 1, "callback-return": 0 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as #17694 (comment)
There are also merge conflicts in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
There are merge conflicts because the two removed test files have changed in the meantime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like 7a79445 was lost in rebase.
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Thanks for that. These conflicts were getting messy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
* main: chore: use jsdoc/no-multi-asterisks with allowWhitespace: true (eslint#17900) chore: fix getting scope in tests (eslint#17899) fix!: Parsing 'exported' comment using parseListConfig (eslint#17675) docs: updated examples of `max-lines` rule (eslint#17898) feat!: Remove valid-jsdoc and require-jsdoc (eslint#17694)
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Remove rule
What changes did you make? (Give an overview)
valid-jsdoc
rule, tests, and docsTODO: Determine if we want to remove
require-jsdoc
too.Fixes #15820
Is there anything you'd like reviewers to focus on?