Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from all commits
9b70e84
90504b7
7a61675
13f6270
e493311
1bb2568
629f691
6276b71
1f36e93
859594f
ec021b5
f9b435f
e04072f
22b9fd6
3e4f310
f5e284d
2b2f99a
79d3eb0
efa307a
de24526
6b18f8a
7f3e808
753bf8d
9bc927b
7d25a7b
ce75fa2
6b9b8ac
0dc4810
adb7972
2b8659b
6657263
73868f4
a64b798
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
?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.
In flat config case,
defaultOptions
have already been merged during the validation so this would merge them again?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.
Aha! Yes, you're right. And there's an additional bug, I think, that default values from schemas aren't being consistently applied in the branch right now. 0dc4810 has a passing unit test for flat config and a failing unit test for legacy config.
I think what needs to happen is moving the validation so it happens once for each config, and with
both meta-based and schema-basededit: only meta-based default options applied. Working on that now.Adding my notes as I found the code flow hard to keep track of without them:
Linter
takes in aconfigType
of type"eslintrc" | "flat"
"eslintrc"
, it callsthis._verifyWithConfigArray
-> ... ->_verifyWithoutProcessors
->runRules
"flat"
, it callsthis._verifyWithFlatConfigArray
-> ... ->_verifyWithFlatConfigArrayAndWithoutProcessors
->runRules
_verifyWithFlatConfigArrayAndWithoutProcessors
,const ruleValidator = new RuleValidator()
calls to avalidate()
method. That uses athis.validators
to build validators fromajv.compile(schema)
. This applies schema default options.validate()
,deepMergeArrays
is called withrule.meta?.defaultOptions
. This applies meta default options.runRules
callsgetRuleOptions(configuredRules[ruleId], rule.meta?.defaultOptions)
. This applies meta default options.With both flat and legacy configs, rule validation happens inside
runRules
.With flat configs, rule validation additionally happens before
runRules
. This earlier validation is applied to the result of merging both meta and schema default options.With legacy configs, only the
runRules
validation is applied. That's why it doesn't apply to schema default options.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.
OK, sorry, I'm thoroughly confused and think I need help here. On
main
,Linter
's unit tests never apply schema defaults - neither for flat nor legacy config. JoshuaKGoldberg@9ccaa85 shows a passing unit test per config type showing schema defaults not being applied.In the flat config code path,
Linter
calls toruleValidator.validate(...)
. That means Ajv's options validation and schema defaulting applies there as well as inflat-config-array.js
.So now I'm unsure what the intended boundaries are supposed to be. Do we want schema defaulting to be moved to happen inside
Linter
now? Is that a breaking change?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.
Just to clarify what happens with schema defaults.
Schema defaults in the top-level array never apply. That is, I believe, an unintentional omission (i.e., a bug) in the code that handles provided options. We could have considered fixing that for v9 if we hadn't decided to switch to
defaultOptions
and eventually drop schema defaults. So, in JoshuaKGoldberg@9ccaa85,default: { inSchema: "from-schema-root" }
is a no-op. On the other hand,inSchema: { default: "from-schema", type: "string" }
can apply, but you'd need to pass"my-rule": ["error", {}]
. This should work in config files, both eslintrc and flat config. This should also work whenLinter
is used directly in flat config mode. But it won't work whenLinter
is used directly in eslintrc mode, because in eslintrc mode Linter doesn't validate rule options, which is probably another unintentional omission. I'm not sure if it's worth fixing that at this point, because eslintrc mode is deprecated and will be dropped in v10.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.
Got it, thanks - that's very helpful.
Just confirming then - is the behavior in the latest commit, adb7972, an acceptable one for v9? That its
Linter
unit test for flat config applies on top of schema defaults, while its unit test for legacy config ignores 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.
I think if we are maintaining the current eslintrc behavior, then we are okay. @mdjermanovic?
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.
ping @mdjermanovic
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?