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: add meta.defaultOptions #17656
base: main
Are you sure you want to change the base?
feat: add meta.defaultOptions #17656
Conversation
Hi @JoshuaKGoldberg!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hi @JoshuaKGoldberg!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
Hi @JoshuaKGoldberg!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
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. |
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. |
Not stale. @JoshuaKGoldberg there's a review comment for you to look into. Thanks. |
Hi @JoshuaKGoldberg!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
Hi @JoshuaKGoldberg!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
Hi @JoshuaKGoldberg!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
Hi @JoshuaKGoldberg!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
Hi @JoshuaKGoldberg!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
Hi @JoshuaKGoldberg!, thanks for the Pull Request The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.
To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page. Read more about contributing to ESLint here |
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. |
tests/lib/linter/linter.js
Outdated
@@ -6739,6 +6762,52 @@ var a = "test2"; | |||
}); | |||
}); | |||
|
|||
describe("options", () => { | |||
it("rules should apply meta.defaultOptions on top of schema defaults", () => { |
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.
Note that this is different from main
. I asked about the code intent: #17656 (comment)
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. |
Waiting for @mdjermanovic to review here. @JoshuaKGoldberg there's a conflict to resolve. |
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. |
I believe it's waiting on re-review from @mdjermanovic? There were a few merge conflicts introduced since the last merge from |
Sorry for the delay. A problem is that eslintrc will still be supported in ESLint v9, and rules should work the same regardless of which config system is used. In flat config, default options are merged before validation. I think this generally makes more sense than validating provided options first and then merging defaults. Ajv also applies schema defaults before validating options. In eslintrc mode, however, as currently implemented, default options are merged after validation (right before running rules). This means that the same configuration for a rule could be valid in one config system but invalid in the other. For example, if this is a rule: {
meta: {
defaultOptions: [{
foo: 42
}],
schema: [{
type: "object",
maxProperties: 2 // allows one or two in addition to "foo"?
}]
},
create() {
return {};
}
} configuration I think we should address this, but there are two possible ways:
I'd be in favor of 1), although it probably requires changes in |
if (validateRule) { | ||
|
||
validateRule(ruleOptions.slice(1)); | ||
const slicedOptions = ruleOptions.slice(1); | ||
const mergedOptions = deepMergeArrays(rule.meta?.defaultOptions, slicedOptions); |
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.
Defaults options wouldn't be applied if the rule has schema: false
?
let ruleOptions = Array.isArray(ruleValue) ? ruleValue : [ruleValue]; | ||
|
||
assertIsRuleSeverity(ruleId, ruleOptions[0]); | ||
let ruleOptions = getRuleOptionsInline(ruleId, ruleValue, rule.meta?.defaultOptions); |
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.
Merging default options at this point would cause a bug when the configuration comment just overrides severity (https://eslint.org/docs/next/use/migrate-to-9.0.0#eslint-comment-options) because for the rest of the code below it would look like the comment had options specified.
We could let ruleValidator.validate()
below merge default options, and then get the final rule config from the config that was passed to it?
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. |
Hi @JoshuaKGoldberg, some reviews are there you can checkout. |
I'm in favor of that as well. Is there some process I should follow? |
@JoshuaKGoldberg nothing special, just go ahead and open a PR on |
I still plan on continuing this work, but likely won't be able to for at least the month of May. ESLint v9 & typescript-eslint v8 are much higher priority for me at the moment. If anybody else wants to tackle the |
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
[x] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Augments linting to factor in a new optional
meta.defaultOptions
. If a rule describes default options this way, any user-provided options are merged on top of the default options.These defaults are applied by a new
deepMergeArrays
utility in:ConfigValidator
Linter
in two places:getRuleOptions
helpergetRuleOptionsInline
helperRuleValidator
insteadRuleValidator
Is there anything you'd like reviewers to focus on?
meta.defaultOptions
on rules rfcs#113