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

Chore: Simplify dataflow in linter.verify #10020

Merged
merged 3 commits into from Feb 26, 2018
Merged

Conversation

@not-an-aardvark
Copy link
Member

not-an-aardvark commented Feb 25, 2018

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

[x] Other, please explain:

What changes did you make? (Give an overview)

This refactors linter.verify to simplify the dataflow throughout the function, and separate out some parts when possible. Specifically:

  • Rules are now run as part of a separate function
  • Environments are now resolved once, as opposed to several times in different places for parserOptions and globals.
  • The defaults for linter.verify are now processed into a consistent object in a separate function.

This should make it easier to implement #9998.

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

Nothing in particular

linter.verify(code, { rules: { foobar: null } });
}, /Invalid config for rule 'foobar'\./);
});
});

This comment has been minimized.

Copy link
@not-an-aardvark

not-an-aardvark Feb 25, 2018

Author Member

I removed this test because we were doing config validation in two separate places: ConfigValidator which verifies all parts of a config including rule schemas, and Linter where we were only checking if a rule's configured value was null and skipping all other validation. The second validation is generally redundant with the first validation.

@not-an-aardvark not-an-aardvark force-pushed the simplify-linter-dataflow branch from 76abcea to 6e6d46d Feb 25, 2018
Copy link
Member

platinumazure left a comment

LGTM, thanks!

Just out of curiosity, what was the effect of the incorrect configuration in conf/environments.js?

@not-an-aardvark

This comment has been minimized.

Copy link
Member Author

not-an-aardvark commented Feb 26, 2018

I think it meant the builtin environment would not work when used in a config file, because it was specified differently from other environments. However, we were always adding the globals from the builtin environment anyway, regardless of configuration.

@not-an-aardvark not-an-aardvark merged commit e4f52ce into master Feb 26, 2018
5 checks passed
5 checks passed
commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@not-an-aardvark not-an-aardvark deleted the simplify-linter-dataflow branch Feb 26, 2018
This was referenced Mar 22, 2018
@renovate renovate bot mentioned this pull request Mar 23, 2018
@eslint eslint bot locked and limited conversation to collaborators Aug 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.