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

Update: config-validator should validate overrides #10357

Merged
merged 1 commit into from
May 28, 2018
Merged

Conversation

mysticatea
Copy link
Member

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

[X] Bug fix

What changes did you make? (Give an overview)

This PR fixes a bug that ESLint overlooks invalid env and rules option values in the overrides option. This is important to catch invalid rule options.

This might be a breaking change since it can throw new error on invalid configs.

Is there anything you'd like reviewers to focus on?

Nothing in particular.

@mysticatea mysticatea added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion labels May 16, 2018
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.

I think this should probably be labeled as a breaking change (and this needs to be discussed in a TSC meeting anyway), but otherwise this LGTM. Thanks!

@mysticatea mysticatea added tsc agenda This issue will be discussed by ESLint's TSC at the next meeting breaking This change is backwards-incompatible labels May 17, 2018
@mysticatea
Copy link
Member Author

TSC Summary: The ConfigValidator#validate overlooks invalid rules and env options in the overrides option. As a result, ESLint doesn't report config errors such as:

{
    "overrides": [{
        "files": "*",
        "rules": {
            "no-undef": ["error", { "InvalidOption": true }]
        }
    }]
}

This PR fixes the bug. This PR is marked as a breaking change since ESLint can throw new config errors on invalid configs.

TSC Question: Should we accept this PR?

@not-an-aardvark
Copy link
Member

I think this is a useful change, but I'm hoping we don't have to delay the 5.0 release for it because we've already received two requests to backport changes.

@mysticatea mysticatea added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed accepted There is consensus among the team that this change meets the criteria for inclusion labels May 17, 2018
@mysticatea mysticatea added this to Needs discussion in v5.0.0 May 17, 2018
@mysticatea
Copy link
Member Author

I'm hoping we don't have to delay the 5.0 release for it

I agree.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

Code LGTM, pending formal acceptance by the TSC.

@platinumazure
Copy link
Member

Please feel free to dismiss my review if needed once TSC agrees to accept this. I only left the "request changes" so that an accidental merge would be harder to do.

@alberto
Copy link
Member

alberto commented May 26, 2018

It's purely semantics at this point, but IMO it's an enhancement. Config validator came before overrides, so we never agreed to validate overrides (which I think we should)

Copy link
Member

@alberto alberto left a comment

Choose a reason for hiding this comment

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

LGTM

@platinumazure
Copy link
Member

Looks like we have "quorum" on this PR. With Ilya on vacation and Nicholas working on his health, it's really just @gyandeeps we're missing here.

@gyandeeps Any objections to this change?

@platinumazure platinumazure dismissed their stale review May 28, 2018 13:42

Allow merge before release

@btmills btmills 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 tsc agenda This issue will be discussed by ESLint's TSC at the next meeting labels May 28, 2018
@btmills btmills merged commit 93c9a52 into master May 28, 2018
v5.0.0 automation moved this from Needs discussion to Done May 28, 2018
@mysticatea mysticatea deleted the config-validator branch June 1, 2018 04:45
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 26, 2018
@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 Nov 26, 2018
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 bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
No open projects
v5.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants