Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(eslint-plugin): fix schemas across several rules and add schema tests#6947
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
fix(eslint-plugin): fix schemas across several rules and add schema tests#6947
Uh oh!
There was an error while loading.Please reload this page.
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 onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedApr 22, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
nx-cloudbot commentedApr 22, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
codecovbot commentedApr 22, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No strong reason: just to dedupe things / keep things in sync.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
bradzacherJun 24, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 there is
eslint/rfcs#103 and the implementing PReslint/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 :) |
JoshuaKGoldberg commentedJul 10, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
This was unintentionally auto-closed when we merged the |
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)