Fix remaining lint errors in `build-system` and enforce lint checks via Travis #11365

rsimha · 2017-09-20T22:30:37Z

Follow up to #11325
Follow up to #11334
Follow up to #11357
Fixes #11273

rsimha · 2017-09-20T23:21:52Z

rsimha · 2017-09-20T23:36:11Z

jridgewell · 2017-09-21T00:14:39Z

build-system/amp.extern.js

@@ -14,6 +14,8 @@
 * limitations under the License.
 */



We shouldn't be linting this file.

jridgewell · 2017-09-21T00:14:55Z

build-system/amp.nti.extern.js

@@ -27,4 +27,4 @@
 * @return {function(number):T}
 * @template T
 */
-var TransitionDef = function(unusedNormtime, unusedCompleted) {};
+const TransitionDef = function(unusedNormtime, unusedCompleted) {};


Shouldn't be linting.

jridgewell · 2017-09-21T00:14:59Z

build-system/amp.oti.extern.js

@@ -24,4 +24,4 @@
 * transition and "false" for ongoing.
 * @typedef {function(number, boolean):?|function(number):?}
 */
-var TransitionDef;
+let TransitionDef;


Shouldn't be linting.

jridgewell · 2017-09-21T00:16:53Z

build-system/app.js

@@ -1199,4 +1198,4 @@ function generateInfo(filePath) {
      '<h3><a href = /serve_mode=cdn>Change to CDN mode (prod JS)</a></h3>';
 }

-module.exports = app;
+module.exports = app;  // eslint-disable-line no-undef


What's undefined?

Linter complains that module is undefined.

Removed after setting env to node.

jridgewell · 2017-09-21T00:17:06Z

build-system/config.js

-    '!build-system/eslint-rules/**/*.*',
-    '!build-system/node_modules/**/*.*',
-  ],
+module.exports = {  // eslint-disable-line no-undef


What's undefined?

Removed after setting env to node.

jridgewell · 2017-09-21T00:18:44Z

build-system/tasks/csvify-size/index.js

@@ -38,12 +38,10 @@ let Tables;
 *   size: string
 * }}
 */
-let Fields;
+let Fields;  // eslint-disable-line no-unused-vars


FieldsDef

jridgewell · 2017-09-21T00:19:25Z

build-system/tasks/dep-check.js

@@ -223,7 +219,7 @@ function flattenGraph(entryPoints) {
  entryPoints = entryPoints.map(entryPoint => entryPoint.deps);
  // Now make the graph have unique entries
  return flatten(entryPoints)
-      .reduce((acc, cur, i, arr) => {
+      .reduce((acc, cur, unusedI, unusedArr) => {


jridgewell · 2017-09-21T00:19:49Z

build-system/tasks/gen-codeowners/index.js

@@ -92,7 +91,7 @@ function generate(root, target, writeToDisk) {
        const dirname = path.relative(process.cwd(), path.dirname(file.path));
        dirs[dirname || '*'] = yaml.parse(file.contents.toString());
      }))
-      .on('end', function(cb) {
+      .on('end', function(unusedCb) {


jridgewell · 2017-09-21T00:20:05Z

build-system/tasks/get-zindex/index.js

@@ -76,7 +76,7 @@ function onFileThrough(file, enc, cb) {
  postcss([zIndexCollector.bind(null, selectors)])
      .process(file.contents.toString(), {
        from: file.relative,
-      }).then(res => {
+      }).then(unusedRes => {


jridgewell · 2017-09-21T00:20:19Z

build-system/tasks/karma.conf.js

@@ -18,7 +18,7 @@
 /**
 * @param {!Object} config
 */
-module.exports = {
+module.exports = {  // eslint-disable-line no-undef


We should set the eslintrc to node environment.

rsimha

@jridgewell all comments addressed. PTAL.

rsimha · 2017-09-21T00:37:51Z

build-system/tasks/changelog.js

@@ -536,7 +536,7 @@ function buildPrMetadata(pr) {
    id: pr.number,
    title: pr.title,
    body: pr.body,
-    merge_commit_sha: pr.merge_commit_sha,
+    merge_commit_sha: pr.merge_commit_sha,  // eslint-disable-line google-camelcase/google-camelcase


I tried quotes. The error is in pr.merge_commit_sha, which appears to be a well defined github constant.

rsimha · 2017-09-21T00:44:15Z

build-system/amp.extern.js

@@ -14,6 +14,8 @@
 * limitations under the License.
 */



rsimha · 2017-09-21T00:44:23Z

build-system/amp.nti.extern.js

@@ -27,4 +27,4 @@
 * @return {function(number):T}
 * @template T
 */
-var TransitionDef = function(unusedNormtime, unusedCompleted) {};
+const TransitionDef = function(unusedNormtime, unusedCompleted) {};


rsimha · 2017-09-21T00:44:31Z

build-system/amp.oti.extern.js

@@ -24,4 +24,4 @@
 * transition and "false" for ongoing.
 * @typedef {function(number, boolean):?|function(number):?}
 */
-var TransitionDef;
+let TransitionDef;


rsimha · 2017-09-21T00:45:30Z

build-system/config.js

-    '!build-system/eslint-rules/**/*.*',
-    '!build-system/node_modules/**/*.*',
-  ],
+module.exports = {  // eslint-disable-line no-undef


rsimha · 2017-09-21T00:50:05Z

build-system/tasks/gen-codeowners/index.js

@@ -92,7 +91,7 @@ function generate(root, target, writeToDisk) {
        const dirname = path.relative(process.cwd(), path.dirname(file.path));
        dirs[dirname || '*'] = yaml.parse(file.contents.toString());
      }))
-      .on('end', function(cb) {
+      .on('end', function(unusedCb) {


rsimha · 2017-09-21T00:50:34Z

build-system/tasks/get-zindex/index.js

@@ -76,7 +76,7 @@ function onFileThrough(file, enc, cb) {
  postcss([zIndexCollector.bind(null, selectors)])
      .process(file.contents.toString(), {
        from: file.relative,
-      }).then(res => {
+      }).then(unusedRes => {


rsimha · 2017-09-21T00:52:07Z

build-system/tasks/karma.conf.js

@@ -18,7 +18,7 @@
 /**
 * @param {!Object} config
 */
-module.exports = {
+module.exports = {  // eslint-disable-line no-undef


rsimha · 2017-09-21T00:57:24Z

build-system/app.js

@@ -1199,4 +1198,4 @@ function generateInfo(filePath) {
      '<h3><a href = /serve_mode=cdn>Change to CDN mode (prod JS)</a></h3>';
 }

-module.exports = app;
+module.exports = app;  // eslint-disable-line no-undef


Removed after setting env to node.

rsimha · 2017-09-21T00:57:31Z

build-system/config.js

-    '!build-system/eslint-rules/**/*.*',
-    '!build-system/node_modules/**/*.*',
-  ],
+module.exports = {  // eslint-disable-line no-undef


Removed after setting env to node.

jridgewell · 2017-09-21T00:59:53Z

build-system/tasks/changelog.js

@@ -536,7 +536,7 @@ function buildPrMetadata(pr) {
    id: pr.number,
    title: pr.title,
    body: pr.body,
-    merge_commit_sha: pr.merge_commit_sha,
+    merge_commit_sha: pr.merge_commit_sha,  // eslint-disable-line google-camelcase/google-camelcase


Both can use quotes.

jridgewell · 2017-09-21T01:00:37Z

build-system/tasks/get-zindex/index.js

@@ -87,11 +87,11 @@ function onFileThrough(file, enc, cb) {
 * @param {function()} cb callback to end the stream
 * @return {!Array<!Array<string>>}
 */
-function createTable(filesData, cb) {
+function createTable(filesData, unusedCb) {


Remove anything that starts with "unused".

Scanned the whole PR, removed all others.

jridgewell · 2017-09-21T01:00:43Z

build-system/tasks/json-check.js

@@ -26,7 +26,7 @@ const through2 = require('through2');
 function checkValidJson() {
  let hasError = false;
  return gulp.src(jsonGlobs)
-      .pipe(through2.obj(function(file, enc, cb) {
+      .pipe(through2.obj(function(file, unusedEnc, unusedCb) {


jridgewell · 2017-09-21T01:00:56Z

build-system/tasks/make-golden.js

@@ -204,7 +204,7 @@ function diffScreenshot_(file, dir, host, verbose, cb) {
          util.log(util.colors.red('Screenshot diff failed: ', file, error));
          cb({error});
        })
-        .on('end', function(res) {
+        .on('end', function(unusedRes) {


jridgewell · 2017-09-21T01:01:33Z

build-system/tasks/release-tagging.js

@@ -155,7 +150,7 @@ function gitFetch(dir) {
      args: 'clone https://github.com/ampproject/amphtml.git',
    });
  }
-  return clonePromise.then(function(arg) {
+  return clonePromise.then(function(unusedArg) {


rsimha

All fixes applied. PTAL @jridgewell

rsimha · 2017-09-21T01:08:18Z

build-system/tasks/csvify-size/index.js

@@ -164,12 +161,13 @@ function mergeTables(dateTimes, tables) {
  // populate all other columns with their respective file size if any.
  dateTimes.forEach(dateTime => {
    // Seed array with empty string values
-    const row = Array.apply(null, Array(tableHeaders[0].length)).map(x => '""');
+    const row =
+        Array.apply(null, Array(tableHeaders[0].length)).map(unusedX => '""');


rsimha · 2017-09-21T01:09:58Z

build-system/tasks/json-check.js

@@ -26,7 +26,7 @@ const through2 = require('through2');
 function checkValidJson() {
  let hasError = false;
  return gulp.src(jsonGlobs)
-      .pipe(through2.obj(function(file, enc, cb) {
+      .pipe(through2.obj(function(file, unusedEnc, unusedCb) {


rsimha · 2017-09-21T01:10:13Z

build-system/tasks/get-zindex/index.js

@@ -87,11 +87,11 @@ function onFileThrough(file, enc, cb) {
 * @param {function()} cb callback to end the stream
 * @return {!Array<!Array<string>>}
 */
-function createTable(filesData, cb) {
+function createTable(filesData, unusedCb) {


Scanned the whole PR, removed all others.

rsimha · 2017-09-21T01:10:38Z

build-system/tasks/make-golden.js

@@ -204,7 +204,7 @@ function diffScreenshot_(file, dir, host, verbose, cb) {
          util.log(util.colors.red('Screenshot diff failed: ', file, error));
          cb({error});
        })
-        .on('end', function(res) {
+        .on('end', function(unusedRes) {


rsimha · 2017-09-21T01:10:53Z

build-system/tasks/release-tagging.js

@@ -155,7 +150,7 @@ function gitFetch(dir) {
      args: 'clone https://github.com/ampproject/amphtml.git',
    });
  }
-  return clonePromise.then(function(arg) {
+  return clonePromise.then(function(unusedArg) {


rsimha · 2017-09-21T01:21:15Z

build-system/tasks/changelog.js

@@ -536,7 +536,7 @@ function buildPrMetadata(pr) {
    id: pr.number,
    title: pr.title,
    body: pr.body,
-    merge_commit_sha: pr.merge_commit_sha,
+    merge_commit_sha: pr.merge_commit_sha,  // eslint-disable-line google-camelcase/google-camelcase


Just the field names needed quotes. The rest is fixed with env being set to node.

rsimha · 2017-09-21T16:25:37Z

@choumx @jridgewell Can I bug one of you to take another look at this? I'd like to merge this soon since any conflicting changes that get merged before this will be a pain to resolve.

dreamofabear · 2017-09-21T16:31:34Z

build-system/app.js

@@ -313,7 +312,7 @@ function proxyToAmpProxy(req, res, mode) {
    body = body
        // Unversion URLs.
        .replace(/https\:\/\/cdn\.ampproject\.org\/rtv\/\d+\//g,
-            'https://cdn.ampproject.org/')
+        'https://cdn.ampproject.org/')


Is this indentation right?

Per our existing rules, it is right. First continuation line is indented by 4 spaces, following lines use the same indentation. This was done automatically by --fix, and wasn't a manual change. If this seems wrong to you, I could look at adding a more refined rule and rerunning the linter in --fix mode across all our files.

Thanks for the review. All your comments have been addressed.

…ia Travis (ampproject#11365)

googlebot added the cla: yes label Sep 20, 2017

rsimha changed the title ~~Manually fix remaining lint errors build-system and enforce lint checks~~ Manually fix remaining lint errors in build-system and enforce lint checks in future Sep 20, 2017

rsimha requested review from jridgewell, erwinmombay and dreamofabear September 20, 2017 23:21

rsimha changed the title ~~Manually fix remaining lint errors in build-system and enforce lint checks in future~~ Fix all lint errors in build-system and enforce lint checks in pr-check.js Sep 20, 2017

rsimha changed the title ~~Fix all lint errors in build-system and enforce lint checks in pr-check.js~~ Fix remaining lint errors in build-system and enforce lint checks via Travis Sep 20, 2017

rsimha requested a review from cramforce September 20, 2017 23:35

jridgewell previously requested changes Sep 21, 2017

View reviewed changes

rsimha commented Sep 21, 2017

View reviewed changes

jridgewell reviewed Sep 21, 2017

View reviewed changes

rsimha commented Sep 21, 2017

View reviewed changes

rsimha added 6 commits September 21, 2017 10:35

Manually fix remaining lint errors

b0d000f

Automatically fix files in root dir of build-system/

2f8801f

Manually fix errors in root of build-system

f8b9578

Get rid of --build_system mode in the lint checker

c67cca7

Address feedback

70082b7

More fixes

75c1529

rsimha mentioned this pull request Sep 21, 2017

Add lint rules for build-system/ #11273

Closed

dreamofabear approved these changes Sep 21, 2017

View reviewed changes

rsimha merged commit ee2dea7 into ampproject:master Sep 21, 2017

rsimha deleted the 2017-09-20-FixRemainingLintErrors branch September 21, 2017 16:55

rsimha mentioned this pull request Sep 25, 2017

Weekly Status 2017-09-25 #11409

Closed

dmvjs pushed a commit to dmvjs/amphtml that referenced this pull request Jan 31, 2018

Fix remaining lint errors in build-system and enforce lint checks v…

9e1b4f2

…ia Travis (ampproject#11365)

Fix remaining lint errors in build-system and enforce lint checks via Travis #11365

Fix remaining lint errors in build-system and enforce lint checks via Travis #11365

Conversation

rsimha commented Sep 20, 2017 • edited

rsimha commented Sep 20, 2017

rsimha commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha commented Sep 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix remaining lint errors in `build-system` and enforce lint checks via Travis #11365

Fix remaining lint errors in `build-system` and enforce lint checks via Travis #11365

rsimha commented Sep 20, 2017 •

edited