Skip to content
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

Validate against invalid rule schema defaults in RuleTester #11473

Closed
not-an-aardvark opened this issue Mar 4, 2019 · 2 comments
Closed

Validate against invalid rule schema defaults in RuleTester #11473

not-an-aardvark opened this issue Mar 4, 2019 · 2 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects

Comments

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Mar 4, 2019

The version of ESLint you are using.

v5.15.0

The problem you want to solve.

As of v5.14.0, we allow rule schemas to include the default keyword to specify default options. However, it's easy for a rule author to inadvertently use the default keyword in a place where the keyword is ignored, resulting in a buggy rule. (Also see: #11427)

Your take on the correct solution to problem.

When we validate rule schemas in RuleTester, we should use the new strictDefaults option in Ajv to raise an error if a rule schema has an invalid default keyword.

This is a breaking change for users who use RuleTester, because RuleTester would sometimes start reporting additional validation errors. However, rules themselves would still work as before, even if they contain invalid defaults. Additionally, the impact is expected to be very low since we only added support for the default keyword recently.

Are you willing to submit a pull request to implement this change?

Yes

@not-an-aardvark not-an-aardvark added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features breaking This change is backwards-incompatible evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Mar 4, 2019
@not-an-aardvark not-an-aardvark added this to Needs discussion in v6.0.0 Mar 4, 2019
@aladdin-add
Copy link
Member

And, we can enable validateSchema: true in rule tester.

eslint/lib/util/ajv.js

Lines 18 to 25 in b00a5e9

const ajv = new Ajv({
meta: false,
useDefaults: true,
validateSchema: false,
missingRefs: "ignore",
verbose: true,
schemaId: "auto"
});

@ilyavolodin ilyavolodin moved this from Needs discussion to Accepted, ready to implement in v6.0.0 Mar 14, 2019
@not-an-aardvark
Copy link
Member Author

This proposal was accepted in yesterday's TSC meeting.

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Mar 15, 2019
@not-an-aardvark not-an-aardvark moved this from Accepted, ready to implement to Implemented, pending review in v6.0.0 Apr 7, 2019
v6.0.0 automation moved this from Implemented, pending review to Done Apr 9, 2019
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Oct 7, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
No open projects
v6.0.0
  
Done
Development

No branches or pull requests

2 participants