Breaking: require meta for fixable rules (fixes #13349)
#14634
Conversation
mdjermanovic
added
core
lib/linter/linter.js
Outdated
| @@ -919,7 +919,7 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserOptions, parser | |||
| } | |||
| const problem = reportTranslator(...args); | |||
|
|
|||
| if (problem.fix && rule.meta && !rule.meta.fixable) { | |||
| if (problem.fix && !(rule.meta && rule.meta.fixable)) { | |||
| throw new Error("Fixable rules should export a `meta.fixable` property."); | |||
There was a problem hiding this comment.
Now that we throw an error in Linter rather than just RuleTester, "must" seems appropriate here. This wording will match #14573 as well. I'd also be fine extending the message to specify "code" and "whitespace" as allowable values.
| throw new Error("Fixable rules should export a `meta.fixable` property."); | |
| throw new Error("Fixable rules must set the `meta.fixable` property."); |
There was a problem hiding this comment.
Good suggestions! I'll fix this.
I'd also be fine extending the message to specify "code" and "whitespace" as allowable values.
Maybe we could also add the validation that the value is indeed "code" or "whitespace"? That wasn't discussed in #13349, and we're currently not using "code"/"whitespace" for anything, but since we're already making a breaking change this might be a good time to align the rules with the specification (and maybe we will use "code"/"whitespace" in the future to optimize autofixing).
There was a problem hiding this comment.
The earliest mention I could find of fixable is #5417 (comment). Since we haven’t started using it in five years, I’m not sure starting to enforce it strictly with a validation error now has a corresponding benefit to make the breaking change worth it.
There was a problem hiding this comment.
The earliest mention I could find of
fixableis #5417 (comment). Since we haven’t started using it in five years, I’m not sure starting to enforce it strictly with a validation error now has a corresponding benefit to make the breaking change worth it.
Agreed!
Changed the error message per suggestions in 7a983f9
eslint-github-bot
bot
added
the
archived due to age
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 autofixing to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:
fixes #13349 (implements the final step 3)
ESLint v7 requires that a rule that produces fixes must provide
meta.fixableproperty, but only if the rule providesmeta. In other words, rules that don't providemetawere allowed to produce fixes.After this change (ESLint v8.0.0), ESLint will require that a rule that produces fixes must provide
metaandmeta.fixable. Otherwise, an error will be thrown in runtime.This applies to legacy-format rules, too. Basically, all fixable rules must be converted to the new format.
What changes did you make? (Give an overview)
Linter.RuleTester(added in Update: requiremetafor fixable rules in RuleTester (refs #13349) #13489 as step 2) because it's no longer necessary.meta.Is there anything you'd like reviewers to focus on?
Is there anything that should be updated in the documentation? I think that wording from #13493 already covers this.