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#6894
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
Uh oh!
There was an error while loading.Please reload this page.
Changes fromall commits
68b193dd4ce93bff0281c168aa06018163cea13617937a8e903102d220ec869a0b89ae2898df4aca2eada97f9a14d6f65eFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -111,8 +111,10 @@ export default util.createRule<Options, MessageIds>({ | ||
| enum: ['array', 'generic', 'array-simple'], | ||
| }, | ||
| }, | ||
| items: [ | ||
JoshuaKGoldberg marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| { | ||
| type: 'object', | ||
| additionalProperties: false, | ||
| properties: { | ||
| default: { | ||
| $ref: '#/$defs/arrayOption', | ||
| @@ -124,7 +126,6 @@ export default util.createRule<Options, MessageIds>({ | ||
| 'The array type expected for readonly cases. If omitted, the value for `default` will be used.', | ||
| }, | ||
| }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Moved this up to the top for consistency with most of the other rules I saw. | ||
| }, | ||
| ], | ||
| type: 'array', | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -9,16 +9,20 @@ const baseRule = getESLintCoreRule('lines-between-class-members'); | ||
| type Options = util.InferOptionsTypeFromRule<typeof baseRule>; | ||
| type MessageIds = util.InferMessageIdsTypeFromRule<typeof baseRule>; | ||
| const schema = Object.values( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Before, we had: {"0":{/* schema A */},"1":{/* schema B */}} AJV is actually not able to interpret this correctly, and no validation actually occurs. This change fixes it to: [{/* schema A */},{/* schema B */}] Using Object.values here should be safe. The baseRule.meta.schema is an array itself, so will be spread in order (guaranteed by ECMA spec). deepMerge will merge the keys in the order of the first object. Object.values will take the values in order, so we'll get back the array in the right order. Member
| ||
| util.deepMerge( | ||
| { ...baseRule.meta.schema }, | ||
| { | ||
| 1: { | ||
| properties: { | ||
| exceptAfterOverload: { | ||
| type: 'boolean', | ||
| default: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| ), | ||
| ); | ||
| export default util.createRule<Options, MessageIds>({ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -51,7 +51,6 @@ export default util.createRule<Options, MessageIds>({ | ||
| 'public readonly', | ||
| ], | ||
| }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Removed this constraint so the default is able to be validated against the schema. Alternative could be to exclude this from the schema validation list, or have an 'override' schema for testing against the schema. I don't think this matters for actual usage, and I think it's not harmful: in fact it might be useful in some configs to be able to specify some allowed things by default, and then specify none to be allowed in an override config explicitly to be clear about why it's there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Sorry, I don't follow - why is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. For this PR, the main reason I did it is beacuse it would prefer the default options from validating against the schema itself, and this is a simple way of getting around that. But also I think semantically it makes sense that it should accept an array of nothing for the exceptions allowed, meaning that no parameter properties are allowed. | ||
| }, | ||
| }, | ||
| additionalProperties: false, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -5,61 +5,160 @@ import type { | ||
| } from 'eslint/lib/rules/no-restricted-imports'; | ||
| import type { Ignore } from 'ignore'; | ||
| import ignore from 'ignore'; | ||
| import type { JSONSchema4 } from 'json-schema'; | ||
| import type { | ||
| InferMessageIdsTypeFromRule, | ||
| InferOptionsTypeFromRule, | ||
| } from '../util'; | ||
| import { createRule } from '../util'; | ||
| import { getESLintCoreRule } from '../util/getESLintCoreRule'; | ||
| const baseRule = getESLintCoreRule('no-restricted-imports'); | ||
| export type Options = InferOptionsTypeFromRule<typeof baseRule>; | ||
| export type MessageIds = InferMessageIdsTypeFromRule<typeof baseRule>; | ||
| // In some versions of eslint, the base rule has a completely incompatible schema | ||
| // This helper function is to safely try to get parts of the schema. If it's not | ||
| // possible, we'll fallback to less strict checks. | ||
| const tryAccess = <T>(getter: () => T, fallback: T): T => { | ||
| try { | ||
| return getter(); | ||
| } catch { | ||
| return fallback; | ||
| } | ||
| }; | ||
| const baseSchema = baseRule.meta.schema as { | ||
| anyOf: [ | ||
| unknown, | ||
| { | ||
| type: 'array'; | ||
| items: [ | ||
| { | ||
| type: 'object'; | ||
| properties: { | ||
| paths: { | ||
| type: 'array'; | ||
| items: { | ||
| anyOf: [ | ||
| { type: 'string' }, | ||
| { | ||
| type: 'object'; | ||
| properties: JSONSchema4['properties']; | ||
| required: string[]; | ||
| }, | ||
| ]; | ||
| }; | ||
| }; | ||
| patterns: { | ||
| anyOf: [ | ||
| { type: 'array'; items: { type: 'string' } }, | ||
| { | ||
| type: 'array'; | ||
| items: { | ||
| type: 'object'; | ||
| properties: JSONSchema4['properties']; | ||
| required: string[]; | ||
| }; | ||
| }, | ||
| ]; | ||
| }; | ||
| }; | ||
| }, | ||
| ]; | ||
| }, | ||
| ]; | ||
| }; | ||
| const allowTypeImportsOptionSchema: JSONSchema4['properties'] = { | ||
| allowTypeImports: { | ||
| type: 'boolean', | ||
| }, | ||
| }; | ||
| const arrayOfStringsOrObjects: JSONSchema4 = { | ||
| type: 'array', | ||
| items: { | ||
| anyOf: [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. These seem incorrect and unnecessary. I think the intention was for this to merge with the base Although thinking about this now, I'm not certain the updated rule is 100% correct, would appreciate careful eyes on it. Might need to refactor how this extends from the base. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Have redone this. It's fairly horrible how it indexes into the baseRule - happy to take feedback on better approaches. Member
| ||
| { type: 'string'}, | ||
| { | ||
| type: 'object', | ||
| properties: { | ||
| ...tryAccess( | ||
| () => | ||
| baseSchema.anyOf[1].items[0].properties.paths.items.anyOf[1] | ||
| .properties, | ||
| undefined, | ||
| ), | ||
| ...allowTypeImportsOptionSchema, | ||
| }, | ||
| required: tryAccess( | ||
| () => | ||
| baseSchema.anyOf[1].items[0].properties.paths.items.anyOf[1] | ||
| .required, | ||
| undefined, | ||
| ), | ||
| }, | ||
| ], | ||
| }, | ||
| uniqueItems: true, | ||
| }; | ||
| const arrayOfStringsOrObjectPatterns: JSONSchema4 = { | ||
| anyOf: [ | ||
| { | ||
| type: 'array', | ||
| items: { | ||
| type: 'string', | ||
| }, | ||
| uniqueItems: true, | ||
| }, | ||
| { | ||
| type: 'array', | ||
| items: { | ||
| type: 'object', | ||
| properties: { | ||
| ...tryAccess( | ||
| () => | ||
| baseSchema.anyOf[1].items[0].properties.patterns.anyOf[1].items | ||
| .properties, | ||
| undefined, | ||
| ), | ||
| ...allowTypeImportsOptionSchema, | ||
| }, | ||
| required: tryAccess( | ||
| () => | ||
| baseSchema.anyOf[1].items[0].properties.patterns.anyOf[1].items | ||
| .required, | ||
| [], | ||
| ), | ||
| }, | ||
| uniqueItems: true, | ||
| }, | ||
| ], | ||
| }; | ||
| const schema: JSONSchema4 = { | ||
| anyOf: [ | ||
| arrayOfStringsOrObjects, | ||
| { | ||
| type: 'array', | ||
| items: [ | ||
| { | ||
| type: 'object', | ||
| properties: { | ||
| paths:arrayOfStringsOrObjects, | ||
| patterns:arrayOfStringsOrObjectPatterns, | ||
| }, | ||
| additionalProperties: false, | ||
| }, | ||
| ], | ||
| additionalItems: false, | ||
| }, | ||
| ], | ||
| }; | ||
| function isObjectOfPaths( | ||
| obj: unknown, | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -52,22 +52,21 @@ export default util.createRule<Options, MessageIds>({ | ||
| ], | ||
| }, | ||
| }, | ||
| items: [ | ||
| { | ||
| type: 'object', | ||
| additionalProperties: false, | ||
| properties: { | ||
| allow: { | ||
| type: 'array', | ||
| items: { | ||
| $ref: '#/$defs/modifier', | ||
| }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Same minItems reasoning as before | ||
| }, | ||
| prefer: { | ||
| enum: ['class-property', 'parameter-property'], | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| type: 'array', | ||
Uh oh!
There was an error while loading.Please reload this page.