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

Fix: no-restricted-syntax - correct the schema #12051

Merged
merged 1 commit into from Aug 12, 2019
Merged

Fix: no-restricted-syntax - correct the schema #12051

merged 1 commit into from Aug 12, 2019

Conversation

bradzacher
Copy link
Contributor

What is the purpose of this pull request? (put an "X" next to item)

[x] Bug fix (template)

What did you do? Please include the actual source code causing the issue.
configured the rule invalidly:

{
  rules: {
    'no-restricted-syntax': ['error', 'FunctionExpression', 1, false, [], {}],
  }
}

What did you expect to happen?
eslint fails with a configuration validation errors

What actually happened? Please include the actual, raw output from ESLint.
the rule ran fine with no errors.


For reference, running the current schema through jsonschemavalidator.net:
image

Running the new schema through jsonschemavalidator.net:
image

Running a lint with the new schema:

Error: .eslintrc.js:
        Configuration for rule "no-restricted-syntax" is invalid:
        Value 1 should be string.
        Value 1 should be object.
        Value 1 should match exactly one schema in oneOf.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Aug 2, 2019
@bradzacher bradzacher changed the title fix: no-restricted-syntax - correct the schema Fix: no-restricted-syntax - correct the schema Aug 2, 2019
Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing!

Is this testable? Answer might be no. But it's always good to see if we could avoid regressions here.

@platinumazure platinumazure added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Aug 2, 2019
@bradzacher
Copy link
Contributor Author

Unfortunately it's not. If you run the rule tester with a config that goes against the schema, it'll just throw an error and fail the test.

Could potentially add tests outside of the standard rule testing tooling - up to you if you think that's a good idea.


I only found the issue because I've been doing some work to automatically generate typescript types based off of the rule's json schema.

I've found rules in other plugins which had similar invalid schemas.

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for contributing!

@platinumazure
Copy link
Member

Thanks @bradzacher for the explanation, as well as digging in and finding these issues. I don't think it's critical to add tests at this time, but other team members may think otherwise.

Copy link
Member

@g-plane g-plane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@bradzacher bradzacher mentioned this pull request Aug 8, 2019
Copy link
Member

@aladdin-add aladdin-add left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

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 bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants