fix(eslint-plugin): fix schemas across several rules and add schema tests #6947
Conversation
|
Thanks for the PR, @domdomegg! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6947 +/- ##
==========================================
- Coverage 87.55% 87.54% -0.01%
==========================================
Files 375 377 +2
Lines 13167 13181 +14
Branches 3899 3899
==========================================
+ Hits 11528 11539 +11
- Misses 1259 1262 +3
Partials 380 380
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| } | ||
| }; | ||
|
|
||
| const baseSchema = baseRule.meta.schema as { |
There was a problem hiding this comment.
question: why did you switch back to basing off of the base rule's schema here?
Any reason or was it just to go back to de-duping things?
There was a problem hiding this comment.
No strong reason: just to dedupe things / keep things in sync.
| describe('schema validation', () => { | ||
| // https://github.com/typescript-eslint/typescript-eslint/issues/6852 | ||
| test("array-type does not accept 'simple-array' option", () => { | ||
| if (areOptionsValid(rule, [{ default: 'simple-array' }])) { | ||
| throw new Error(`Options succeeded validation for bad options`); | ||
| } | ||
| }); | ||
|
|
||
| // https://github.com/typescript-eslint/typescript-eslint/issues/6892 | ||
| test('array-type does not accept non object option', () => { | ||
| if (areOptionsValid(rule, ['array'])) { | ||
| throw new Error(`Options succeeded validation for bad options`); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I wonder if we should do a rule-tester style API here to remove the boilerplate.
For example something like this:
ruleOptionsTester(rule, {
valid: [ ... ],
invalid: [
[{ default: 'simple-array' }],
['array'],
],
});There was a problem hiding this comment.
Yeah, I think something like this would be neat if we end up doing a lot of these kinds of tests. For now I think it's simple enough that we can leave this to be done in future if necessary?
There was a problem hiding this comment.
Note that there is
eslint/rfcs#103 and the implementing PR eslint/eslint#16823
This would allow us to include these tests in the rule like:
tester.run('rule', rule, {
valid: [ ... ],
invalid: [ ... ],
fatal: [
{
options: ['simple-array'],
error: {
name: 'SchemaValidationError',
message: 'Whatever the json schema error is',
},
},
{
options: ['array'],
error: {
name: 'SchemaValidationError',
message: 'Whatever the json schema error is',
},
},
],
}so definitely we don't need to bother with building a bespoke test framework!
|
Removing from the v6 milestone as it doesn't actually block releasing v6 (I believe). Someone yell at me if that's wrong. 🙂 |
|
Oh also, @domdomegg just checking in - are you waiting on us for anything? I believe our last status was waiting on you to re-request review to take another look. |
|
@domdomegg if you want to update this against |
|
I think we're ready @bradzacher / @JoshuaKGoldberg :) |
|
This was unintentionally auto-closed when we merged the |
The base branch was changed.
|
Thanks @JoshuaKGoldberg 🙌 |
PR Checklist
simple-arrayshould be disallowed by the schema #6852, Bug: [array-type] Does not validate shape of config #6892Overview
Rebase of #6894 on top of v6, as per #6894 (comment)