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 gulp-eslint version to 4.0 #12450

Merged
merged 4 commits into from
Dec 13, 2017
Merged

Update gulp-eslint version to 4.0 #12450

merged 4 commits into from
Dec 13, 2017

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Dec 13, 2017

This PR does the following:

  1. Updates the version of gulp-eslint used by gulp lint to 4.0
  2. Removes the deprecated ecmaFeatures section of .eslintrc (we've already set ecmaVersion to 6) (See https://eslint.org/docs/user-guide/migrating-to-2.0.0#language-options)
  3. Fixes several indentation errors detected by a more strict indent rule (See https://eslint.org/docs/user-guide/migrating-to-4.0.0#indent-rewrite)
  4. Fixes several comment spacing errors detected by a more strict no-multi-spaces rule (See https://eslint.org/docs/user-guide/migrating-to-4.0.0#-the-no-multi-spaces-rule-is-more-strict-by-default)

Partial fix for #12181
Follow up to #12350

@rsimha
Copy link
Contributor Author

rsimha commented Dec 13, 2017

/to @aghassemi @choumx @erwinmombay

@rsimha
Copy link
Contributor Author

rsimha commented Dec 13, 2017

For a more sane code review without whitespace-only changes, use this link: https://github.com/ampproject/amphtml/pull/12450/files?w=1

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

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

My only comment is about making sure that gulp lint still picks up custom rules defined in elint-rules folder, my worry is that maybe gulp-eslint v4 has followed the path of eslint v4 and removed support for rulesPath in favour of either rulesdir or plugins.

One way to verify is using the banned spread operator. In any file change a function call like foo(bar); to foo(...bar); and run gulp lint if it does not complain, custom rules are not being picked up.

@rsimha
Copy link
Contributor Author

rsimha commented Dec 13, 2017

@aghassemi The rules in eslint-rules do get picked up. I've verified this by running

DEBUG=eslint:cli-engine gulp lint

Output:

~/src/amphtml$ DEBUG=eslint:cli-engine gulp lint
[18:49:25] Using gulpfile ~/src/amphtml/gulpfile.js
[18:49:25] Starting 'lint'...
  eslint:cli-engine Loading rules from build-system/eslint-rules/ +0ms
  eslint:cli-engine Linting /usr/local/google/home/rsimha/src/amphtml/3p/3p.js +752ms
  eslint:cli-engine Linting /usr/local/google/home/rsimha/src/amphtml/3p/ampcontext-integration.js +1s
  eslint:cli-engine Linting /usr/local/google/home/rsimha/src/amphtml/3p/ampcontext-lib.js +458ms
  ...
  eslint:cli-engine Linting /usr/local/google/home/rsimha/src/amphtml/extensions/amp-viewer-integration/0.1/test/integration/test-amp-viewer-integration.js +36ms
  eslint:cli-engine Linting /usr/local/google/home/rsimha/src/amphtml/extensions/amp-viewer-integration/0.1/test/integration/test-amp-webview-viewer-integration.js +78ms
  eslint:cli-engine Linting /usr/local/google/home/rsimha/src/amphtml/extensions/amp-viewer-integration/0.1/test/integration/test-viewer-initiated-handshake.js +18ms
[18:51:09] Finished 'lint' after 39 s

@rsimha
Copy link
Contributor Author

rsimha commented Dec 13, 2017

Double-verified by making sure that adding a spread operator will result in a lint error :)

@aghassemi
Copy link
Contributor

Thanks @rsimha-amp . LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants