Skip to content

Update gulp-eslint version to 4.0#12450

Merged
rsimha merged 4 commits intoampproject:masterfrom
rsimha:2017-12-13-GulpEslint
Dec 13, 2017
Merged

Update gulp-eslint version to 4.0#12450
rsimha merged 4 commits intoampproject:masterfrom
rsimha:2017-12-13-GulpEslint

Conversation

@rsimha
Copy link
Copy Markdown
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 rsimha self-assigned this Dec 13, 2017
@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Dec 13, 2017

/to @aghassemi @choumx @erwinmombay

@rsimha
Copy link
Copy Markdown
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

@rsimha rsimha mentioned this pull request Dec 13, 2017
Merged
aghassemi
aghassemi reviewed Dec 13, 2017
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

rsimha commented Dec 13, 2017 •
edited
Loading

@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
Copy Markdown
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
Copy Markdown
Contributor

aghassemi commented Dec 13, 2017

Thanks @rsimha-amp . LGTM.

@rsimha rsimha merged commit 53e8123 into ampproject:master Dec 13, 2017
@rsimha rsimha deleted the 2017-12-13-GulpEslint branch December 13, 2017 23:56
This was referenced Dec 18, 2017
Merged
Merged
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
@rsimha @gzgogo
Update gulp-eslint version to 4.0 (ampproject#12450)
a714a9f
@ampprojectbot ampprojectbot mentioned this pull request Jun 16, 2020
Closed
@ampprojectbot ampprojectbot mentioned this pull request Jul 30, 2020
Closed
@ampprojectbot ampprojectbot mentioned this pull request Sep 3, 2020
Closed
@ampprojectbot ampprojectbot mentioned this pull request Mar 4, 2021
Open
This was referenced May 21, 2021
Closed
Closed
This was referenced Jul 15, 2021
Closed
Closed
@ampprojectbot ampprojectbot mentioned this pull request Aug 27, 2021
Closed
@ampprojectbot ampprojectbot mentioned this pull request Nov 8, 2021
Open
@ampprojectbot ampprojectbot mentioned this pull request Jan 20, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erwinmombay erwinmombay Awaiting requested review from erwinmombay

@dreamofabear dreamofabear Awaiting requested review from dreamofabear

1 more reviewer

@aghassemi aghassemi aghassemi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rsimha rsimha

Labels

cla: yes PR use: In Beta / Experimental WG: infra

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rsimha @aghassemi @googlebot @ampprojectbot