[grunt/eslint] fix precommit linting by spalger · Pull Request #9510 · elastic/kibana

spalger · 2016-12-15T19:06:01Z

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

- 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

epixa · 2016-12-15T20:36:46Z

tasks/lint_staged_files.js

      }

      if (cli.isPathIgnored(file)) {
+        skippingFile(file, 'ignored by .eslintignore');


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 · 2016-12-15T20:38:41Z

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');


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?

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

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 · 2016-12-15T21:46:10Z

@epixa that should address your feedback

epixa · 2016-12-16T19:17:25Z

LGTM

ppisljar

LGTM

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 <spalger@users.noreply.github.com> on 2016-12-15T19:01:21Z **Commit 2:** [grunt/eslint] only log warning wtr grunt paths * Original sha: b152e35 * Authored by spalger <spalger@users.noreply.github.com> on 2016-12-15T21:45:40Z

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

## Dependency updates - `@elastic/eui` - v113.3.0 ⏩ v114.0.0 - `@elastic/eui-theme-borealis` - v6.2.0 ⏩ v7.0.0 - `@elastic/eslint-plugin-eui` - v2.10.0 ⏩ v2.11.0 --- ## Package updates ### [`v114.0.0`](https://github.com/elastic/eui/releases/v114.0.0) - Fixed the clipping of `EuiFlyout` overlay mask to the container bounds when the `container` prop is provided, so the mask no longer covers the full viewport for app-scoped flyouts. ([#9512](elastic/eui#9512)) - Updated `EuiFlyout` to support `pushAnimation` prop for `type="overlay"` ([#9428](elastic/eui#9428)) - Added `hasAnimation` prop on `EuiFlyout` (replaces `pushAnimation`) ([#9428](elastic/eui#9428)) - Added `hasAnimation` prop on `EuiOverlayMask` to conditionally add animation styles ([#9428](elastic/eui#9428)) - Added `historyKey` prop (type `symbol`) to `EuiFlyout` and the flyout manager API to support scoped flyout history. ([#9413](elastic/eui#9413)) - Only flyouts sharing the same `Symbol` reference share Back button navigation and history entries; omitting `historyKey` gives each session its own isolated history group. - `ACTION_CLOSE_ALL` now closes only the current history group rather than all open flyouts. **Bug fixes** - Fixed `EuiTreeView` expanded nodes clipping content and causing sibling overlap when children exceed viewport height ([#9510](elastic/eui#9510)) - Fixed `EuiDataGrid` scroll bouncing back to the focused element in certain cases ([#9453](elastic/eui#9453)) - Fixed support for intraword underscores in `EuiMarkdownFormat` ([#9408](elastic/eui#9408)) **Deprecations** - Deprecated `pushAnimation` prop on `EuiFlyout`. Use `hasAnimation` instead. ([#9428](elastic/eui#9428)) **Breaking changes** - Removed `severity.assistance` color token ([#9507](elastic/eui#9507)) - Removed assistance datavis color tokens: ([#9507](elastic/eui#9507)) - `vis.euiColorVisAssistance` - `vis.euiColorVis10` - `vis.euiColorVis11` - `vis.euiColorVisText10` - `vis.euiColorVisText11` - `vis.euiColorVisBehindText10` - `vis.euiColorVisBehindText11` - The positional signature of `FlyoutManagerApi.addFlyout` and the flyout store's `addFlyout` now includes `historyKey` before the `iconType`/`minWidth` arguments. Call sites that pass arguments positionally must be updated (or switched to named parameters) to account for this new parameter ordering. ([#9413](elastic/eui#9413)) **Accessibility** - Fixed `aria-label` not being applied to `EuiColorPicker`'s input element ([#9436](elastic/eui#9436)) ### @elastic/eui-theme-borealis v7.0.0 **Breaking changes** - Removed `severity.assistance` color token ([#9507](elastic/eui#9507)) - Removed assistance datavis color tokens: ([#9507](elastic/eui#9507)) - `vis.euiColorVisAssistance` - `vis.euiColorVis10` - `vis.euiColorVis11` - `vis.euiColorVisText10` - `vis.euiColorVisText11` - `vis.euiColorVisBehindText10` - `vis.euiColorVisBehindText11` ### @elastic/eslint-plugin-eui v2.11.0 - Updated `no-unnamed-interactive-element` to include checking `EuiColorPicker` ([#9436](elastic/eui#9436)) --------- Co-authored-by: Timothy Sullivan <tsullivan@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Weronika Olejniczak <32842468+weronikaolejniczak@users.noreply.github.com> Co-authored-by: Weronika Olejniczak <weronika.olejniczak@elastic.co>

## Dependency updates - `@elastic/eui` - v113.3.0 ⏩ v114.0.0 - `@elastic/eui-theme-borealis` - v6.2.0 ⏩ v7.0.0 - `@elastic/eslint-plugin-eui` - v2.10.0 ⏩ v2.11.0 --- ## Package updates ### [`v114.0.0`](https://github.com/elastic/eui/releases/v114.0.0) - Fixed the clipping of `EuiFlyout` overlay mask to the container bounds when the `container` prop is provided, so the mask no longer covers the full viewport for app-scoped flyouts. ([elastic#9512](elastic/eui#9512)) - Updated `EuiFlyout` to support `pushAnimation` prop for `type="overlay"` ([elastic#9428](elastic/eui#9428)) - Added `hasAnimation` prop on `EuiFlyout` (replaces `pushAnimation`) ([elastic#9428](elastic/eui#9428)) - Added `hasAnimation` prop on `EuiOverlayMask` to conditionally add animation styles ([elastic#9428](elastic/eui#9428)) - Added `historyKey` prop (type `symbol`) to `EuiFlyout` and the flyout manager API to support scoped flyout history. ([elastic#9413](elastic/eui#9413)) - Only flyouts sharing the same `Symbol` reference share Back button navigation and history entries; omitting `historyKey` gives each session its own isolated history group. - `ACTION_CLOSE_ALL` now closes only the current history group rather than all open flyouts. **Bug fixes** - Fixed `EuiTreeView` expanded nodes clipping content and causing sibling overlap when children exceed viewport height ([elastic#9510](elastic/eui#9510)) - Fixed `EuiDataGrid` scroll bouncing back to the focused element in certain cases ([elastic#9453](elastic/eui#9453)) - Fixed support for intraword underscores in `EuiMarkdownFormat` ([elastic#9408](elastic/eui#9408)) **Deprecations** - Deprecated `pushAnimation` prop on `EuiFlyout`. Use `hasAnimation` instead. ([elastic#9428](elastic/eui#9428)) **Breaking changes** - Removed `severity.assistance` color token ([elastic#9507](elastic/eui#9507)) - Removed assistance datavis color tokens: ([elastic#9507](elastic/eui#9507)) - `vis.euiColorVisAssistance` - `vis.euiColorVis10` - `vis.euiColorVis11` - `vis.euiColorVisText10` - `vis.euiColorVisText11` - `vis.euiColorVisBehindText10` - `vis.euiColorVisBehindText11` - The positional signature of `FlyoutManagerApi.addFlyout` and the flyout store's `addFlyout` now includes `historyKey` before the `iconType`/`minWidth` arguments. Call sites that pass arguments positionally must be updated (or switched to named parameters) to account for this new parameter ordering. ([elastic#9413](elastic/eui#9413)) **Accessibility** - Fixed `aria-label` not being applied to `EuiColorPicker`'s input element ([elastic#9436](elastic/eui#9436)) ### @elastic/eui-theme-borealis v7.0.0 **Breaking changes** - Removed `severity.assistance` color token ([elastic#9507](elastic/eui#9507)) - Removed assistance datavis color tokens: ([elastic#9507](elastic/eui#9507)) - `vis.euiColorVisAssistance` - `vis.euiColorVis10` - `vis.euiColorVis11` - `vis.euiColorVisText10` - `vis.euiColorVisText11` - `vis.euiColorVisBehindText10` - `vis.euiColorVisBehindText11` ### @elastic/eslint-plugin-eui v2.11.0 - Updated `no-unnamed-interactive-element` to include checking `EuiColorPicker` ([elastic#9436](elastic/eui#9436)) --------- Co-authored-by: Timothy Sullivan <tsullivan@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Weronika Olejniczak <32842468+weronikaolejniczak@users.noreply.github.com> Co-authored-by: Weronika Olejniczak <weronika.olejniczak@elastic.co>

spalger added review v5.2.0 v6.0.0 labels Dec 15, 2016

spalger assigned epixa and ppisljar Dec 15, 2016

spalger requested review from epixa and ppisljar December 15, 2016 19:06

epixa reviewed Dec 15, 2016

View reviewed changes

[grunt/eslint] only log warning wtr grunt paths

b152e35

jbudz self-requested a review December 16, 2016 19:13

jbudz approved these changes Dec 16, 2016

View reviewed changes

epixa approved these changes Dec 16, 2016

View reviewed changes

spalger merged commit 654a3be into elastic:master Dec 16, 2016

ppisljar reviewed Dec 16, 2016

View reviewed changes

elastic-jasper mentioned this pull request Dec 16, 2016

[5.x] [grunt/eslint] fix precommit linting #9532

Merged

epixa added the Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// label Dec 16, 2016

spalger deleted the fix/eslint-grunt-task branch October 18, 2019 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[grunt/eslint] fix precommit linting#9510

[grunt/eslint] fix precommit linting#9510
spalger merged 2 commits intoelastic:masterfrom
spalger:fix/eslint-grunt-task

spalger commented Dec 15, 2016 •

edited

Loading

Uh oh!

epixa Dec 15, 2016 •

edited

Loading

Uh oh!

epixa Dec 15, 2016

Uh oh!

spalger Dec 15, 2016

Uh oh!

epixa Dec 15, 2016

Uh oh!

spalger commented Dec 15, 2016

Uh oh!

epixa commented Dec 16, 2016

Uh oh!

ppisljar left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

spalger commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

epixa Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epixa Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

spalger Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

epixa Dec 15, 2016

Choose a reason for hiding this comment

Uh oh!

spalger commented Dec 15, 2016

Uh oh!

epixa commented Dec 16, 2016

Uh oh!

ppisljar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

spalger commented Dec 15, 2016 •

edited

Loading

epixa Dec 15, 2016 •

edited

Loading