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.

Sign up for GitHub

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

Jump to bottom

[grunt/eslint] fix precommit linting #9510

Merged
merged 2 commits into from
Dec 16, 2016
Merged

[grunt/eslint] fix precommit linting #9510

merged 2 commits into from
Dec 16, 2016

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Dec 15, 2016 •
edited
Loading

The precommit task is supposed to filter out changed files so that only files covered by the master eslint paths list which are not ignored by the .eslintignore file are sent to eslint.

Those checks are broken since we upgraded eslint and all files are being ignored. To make matters worse, the logging added to the collectFilesToCommit task makes it look like everything is good.

fixed these problems by:

  • removing the use of minimatch.makeRe() because it does not support the entire glob syntax
  • log a warning whenever a js file is excluded by the lintStagedFiles task
  • check for matches against relative file paths, to match eslint's expectation

@spalger
[grunt/eslint] fix precommit linting
ca45ae2 
 - remove use of `minimatch.makeRe()` because it does not support the entire glob syntax
 - log a warning whenever a js file is excluded by the `lintStagedFiles` task
 - eslint globs are relative to the project root, ensure that we check against relative version
@spalger spalger requested review from epixa and ppisljar December 15, 2016 19:06
epixa
epixa reviewed Dec 15, 2016
View reviewed changes
tasks/lint_staged_files.js
return false;
}

if (cli.isPathIgnored(file)) {
skippingFile(file, 'ignored by .eslintignore');
Copy link
Contributor

@epixa epixa Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this case should be a warning. A warning implies something is wrong, but eslintignore applying to a given file is certainly an expected scenario.

Edit: An INFO message or something is probably more appropriate.

epixa
epixa reviewed Dec 15, 2016
View reviewed changes
tasks/lint_staged_files.js
.filter(file => {
if (!sourcePathRegexps.some(re => file.match(re))) {
if (!sourcePathGlobs.some(glob => minimatch(file, glob))) {
skippingFile(file, 'not selected by grunt eslint config');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm on the fence about whether this should be a warning or not, but I'm cool with giving it a go to see it in action for awhile.

This does bring up a good question for me: why not eslint everything and then rely on the blacklisting in eslint for exceptions rather than the combined whitelist/blacklist approach we're using now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

switching to a more comprehensive blacklist vs a combination is a good idea

Copy link
Contributor

Choose a reason for hiding this comment

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

Beyond the scope of this though since the precommit thing is currently broken. Just leave it as a warning for now and create an issue for it.

@spalger
Copy link
Contributor Author

spalger commented Dec 15, 2016

@epixa that should address your feedback

@jbudz jbudz self-requested a review December 16, 2016 19:13
@epixa
Copy link
Contributor

epixa commented Dec 16, 2016

LGTM

@spalger spalger merged commit 654a3be into elastic:master Dec 16, 2016
ppisljar
ppisljar reviewed Dec 16, 2016
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

elastic-jasper added a commit that referenced this pull request Dec 16, 2016
@elastic-jasper
[grunt/eslint] fix precommit linting
0b7deea 
Backports PR #9510

**Commit 1:**
[grunt/eslint] fix precommit linting

 - remove use of `minimatch.makeRe()` because it does not support the entire glob syntax
 - log a warning whenever a js file is excluded by the `lintStagedFiles` task
 - eslint globs are relative to the project root, ensure that we check against relative version

* Original sha: ca45ae2
* Authored by spalger <[email protected]> on 2016-12-15T19:01:21Z

**Commit 2:**
[grunt/eslint] only log warning wtr grunt paths

* Original sha: b152e35
* Authored by spalger <[email protected]> on 2016-12-15T21:45:40Z
@elastic-jasper elastic-jasper mentioned this pull request Dec 16, 2016
Merged
@epixa epixa added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Dec 16, 2016
spalger pushed a commit that referenced this pull request Dec 16, 2016
@elastic-jasper @spalger
[grunt/eslint] fix precommit linting (#9532)
aa41e38 
Backports PR #9510

**Commit 1:**
[grunt/eslint] fix precommit linting

 - remove use of `minimatch.makeRe()` because it does not support the entire glob syntax
 - log a warning whenever a js file is excluded by the `lintStagedFiles` task
 - eslint globs are relative to the project root, ensure that we check against relative version

* Original sha: ca45ae2
* Authored by spalger <[email protected]> on 2016-12-15T19:01:21Z

**Commit 2:**
[grunt/eslint] only log warning wtr grunt paths

* Original sha: b152e35
* Authored by spalger <[email protected]> on 2016-12-15T21:45:40Z
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Jan 19, 2017
@stacey-gammon
View edit mode
506734c 
clean up

show spy panel in embed mode

[webpack] fix loader query string usage (elastic#9497)

* [webpack] pin to fork with fixed loader aliases

* [optimizer] upgrade to postcss+autoprefixer

* [timelion] convert uiExports.modules -> webpackShims

* [uiExports] remove implementation-leaking and unused uiExport types

* [optimizer] remove unused imports

* [uiBundlerEnv] add a method for exporting global import aliases for special cases

[dev tools] Hide app link when there are no tools (elastic#9489)

* [dev tools] Hide app link when there are no tools

* [dev tools] Add tests for setting app as hidden

pie chart unhandled error fix (elastic#9422)

Add NoResults and Panel components. (elastic#9516)

* Add NoResults and Panel components.

* Lighten noResults text.

Update ToolBarFooter component to support content on the left side. (elastic#9514)

Fix bug with Button component appearance inside of a ToolBar. (elastic#9526)

Make basic Button hover state the same both in and out of ToolBar. (elastic#9528)

[grunt/eslint] fix precommit linting (elastic#9510)

* [grunt/eslint] fix precommit linting

 - remove use of `minimatch.makeRe()` because it does not support the entire glob syntax
 - log a warning whenever a js file is excluded by the `lintStagedFiles` task
 - eslint globs are relative to the project root, ensure that we check against relative version

* [grunt/eslint] only log warning wtr grunt paths

Add Tabs component. (elastic#9536)

- Fix bugs with Button and CheckBox focused states.
- Fix appearance of cell content in Table.

Disable linting for Tabs component example JS. (elastic#9538)

Set Button component to display: inline-block, to ensure it has the same height when applied to both button elements and anchor tags. (elastic#9541)

fixing metric vis to correctly show scrollbars when overflown (elastic#9481)

Adding Safari 7 support to autoprefixer (elastic#9534)

PhantomJS is using a rather outdated version of WebKit, which requires
various css-prefixes to render correctly. PhantomJS doesn't have a specific
user-agent, and Safari 7 is the closet version of WebKit.

use Stop Editing instead of Preview

Warn the user if they Stop Editing with unsaved changes

- Refresh the dashboard after stop editing so unsaved changes are lost
and no temporary edits will be shown in non-edit mode.

Don't watch the variable on scope, but the config attribute
@spalger spalger deleted the fix/eslint-grunt-task branch October 18, 2019 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppisljar ppisljar ppisljar left review comments

@jbudz jbudz jbudz approved these changes

@epixa epixa epixa approved these changes

Assignees

@epixa epixa

@ppisljar ppisljar

Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v5.2.0 v6.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@spalger @epixa @jbudz @ppisljar