ampproject · rsimha · Oct 17, 2019 · Oct 16, 2019 · Oct 17, 2019 · Oct 17, 2019
diff --git a/build-system/common/default-pre-push b/build-system/common/default-pre-push
@@ -26,6 +26,7 @@ GREEN() { echo -e "\033[0;32m$1\033[0m"; }
 CYAN() { echo -e "\033[0;36m$1\033[0m"; }
 
 GULP_LINT_LOCAL="gulp lint --local_changes"
+GULP_PRETTIFY_LOCAL="gulp prettify --local_changes"
 GULP_UNIT_LOCAL="gulp unit --local_changes --headless"
 
 echo $(GREEN "Running") $(CYAN "pre-push") $(GREEN "hooks. (Run") $(CYAN "git push --no-verify") $(GREEN "to skip them.)")
@@ -35,6 +36,10 @@ echo $(GREEN "Running") $(CYAN "$GULP_LINT_LOCAL")
 eval $GULP_LINT_LOCAL || exit 1
 echo -e "\n"
 
+echo $(GREEN "Running") $(CYAN "$GULP_PRETTIFY_LOCAL")
+eval $GULP_PRETTIFY_LOCAL || exit 1
+echo -e "\n"
+
 echo $(GREEN "Running") $(CYAN "$GULP_UNIT_LOCAL")
 eval $GULP_UNIT_LOCAL || exit 1
 echo -e "\n"

diff --git a/build-system/common/utils.js b/build-system/common/utils.js
@@ -14,11 +14,15 @@
  * limitations under the License.
  */
 
+const fs = require('fs-extra');
+const globby = require('globby');
 const log = require('fancy-log');
+const {gitDiffNameOnlyMaster} = require('../common/git');
 const {isTravisBuild} = require('../common/travis');
 
 /**
  * Logs a message on the same line to indicate progress
+ *
  * @param {string} message
  */
 function logOnSameLine(message) {
@@ -30,6 +34,24 @@ function logOnSameLine(message) {
   log(message);
 }
 
+/**
+ * Gets the list of files changed on the current branch that match the given
+ * array of glob patterns
+ *
+ * @param {!Array<string>} globs
+ * @return {!Array<string>}
+ */
+function getFilesChanged(globs) {
+  const allFiles = globby.sync(globs);
+  return gitDiffNameOnlyMaster().filter(changedFile => {
+    return (
+      fs.existsSync(changedFile) &&
+      allFiles.some(file => file.endsWith(changedFile))
+    );
+  });
+}
+
 module.exports = {
+  getFilesChanged,
   logOnSameLine,
 };
diff --git a/build-system/tasks/lint.js b/build-system/tasks/lint.js
@@ -21,15 +21,14 @@ const config = require('../test-configs/config');
 const deglob = require('globs-to-files');
 const eslint = require('gulp-eslint');
 const eslintIfFixed = require('gulp-eslint-if-fixed');
-const fs = require('fs-extra');
 const gulp = require('gulp');
 const lazypipe = require('lazypipe');
 const log = require('fancy-log');
 const path = require('path');
 const watch = require('gulp-watch');
+const {getFilesChanged, logOnSameLine} = require('../common/utils');
 const {gitDiffNameOnlyMaster} = require('../common/git');
 const {isTravisBuild} = require('../common/travis');
-const {logOnSameLine} = require('../common/utils');
 const {maybeUpdatePackages} = require('./update-packages');
 
 const isWatching = argv.watch || argv.w || false;
@@ -149,21 +148,6 @@ function runLinter(stream, options) {
     .pipe(eslint.failAfterError());
 }
 
-/**
- * Extracts the list of lintable files in this PR from the commit log.
- *
- * @return {!Array<string>}
- */
-function lintableFilesChanged() {
-  const lintableFiles = deglob.sync(config.lintGlobs);
-  return gitDiffNameOnlyMaster().filter(function(file) {
-    return (
-      fs.existsSync(file) &&
-      lintableFiles.some(lintableFile => lintableFile.endsWith(file))
-    );
-  });
-}
-
 /**
  * Checks if there are eslint rule changes, in which case we must lint all
  * files.
@@ -211,7 +195,7 @@ function lint() {
   if (argv.files) {
     filesToLint = getFilesToLint(argv.files.split(','));
   } else if (!eslintRulesChanged() && argv.local_changes) {
-    const lintableFiles = lintableFilesChanged();
+    const lintableFiles = getFilesChanged(config.lintGlobs);
     if (lintableFiles.length == 0) {
       log(colors.green('INFO: ') + 'No JS files in this PR');
       return Promise.resolve();
@@ -230,6 +214,7 @@ lint.description = 'Validates against Google Closure Linter';
 lint.flags = {
   'watch': '  Watches for changes in files, validates against the linter',
   'fix': '  Fixes simple lint errors (spacing etc)',
+  'files': '  Lints just the specified files',
   'local_changes': '  Lints just the files changed in the local branch',
   'quiet': '  Suppress warnings from outputting',
 };
diff --git a/build-system/tasks/pr-check.js b/build-system/tasks/pr-check.js
@@ -57,7 +57,7 @@ async function prCheck(cb) {
   printChangeSummary(FILENAME);
   const buildTargets = determineBuildTargets(FILENAME);
   runCheck('gulp lint --local_changes');
-  runCheck('gulp prettify');
+  runCheck('gulp prettify --local_changes');
   runCheck('gulp presubmit');
   runCheck('gulp check-exact-versions');
 

diff --git a/build-system/tasks/prettify.js b/build-system/tasks/prettify.js
@@ -23,15 +23,15 @@
 'use strict';
 
 const argv = require('minimist')(process.argv.slice(2));
-const gulp = require('gulp');
+const globby = require('globby');
 const log = require('fancy-log');
 const path = require('path');
-const tap = require('gulp-tap');
+const {getFilesChanged, logOnSameLine} = require('../common/utils');
 const {getOutput} = require('../common/exec');
 const {green, cyan, red, yellow} = require('ansi-colors');
 const {highlight} = require('cli-highlight');
 const {isTravisBuild} = require('../common/travis');
-const {logOnSameLine} = require('../common/utils');
+const {maybeUpdatePackages} = require('./update-packages');
 const {prettifyGlobs} = require('../test-configs/config');
 
 const rootDir = path.dirname(path.dirname(__dirname));
@@ -48,43 +48,75 @@ const PrettierResult = {
 };
 
 /**
- * Checks files for formatting (and optionally fixes them) with Prettier.
- * Returns the cumulative result to the `gulp` process via  process.exitCode so
- * that all files can be checked / fixed.
+ * Logs the list of files that will be checked.
  *
- * @return {!Promise}
+ * @param {!Array<string>} files
  */
-function prettify() {
-  const filesToCheck = argv.files ? argv.files.split(',') : prettifyGlobs;
-  return gulp
-    .src(filesToCheck)
-    .pipe(
-      tap(file => {
-        checkFile(path.relative(rootDir, file.path));
-      })
-    )
-    .on('finish', () => {
-      if (process.exitCode == 1) {
-        logOnSameLine(red('ERROR: ') + 'Found errors in one or more files.');
-        if (!argv.fix) {
-          log(
-            yellow('NOTE 1:'),
-            'You may be able to automatically fix some errors by running',
-            cyan('gulp prettify --fix'),
-            'from your local branch.'
-          );
-          log(
-            yellow('NOTE 2:'),
-            'Since this is a destructive operation (that edits your files',
-            'in-place), make sure you commit before running the command.'
-          );
-        }
-      } else {
-        if (!isTravisBuild()) {
-          logOnSameLine(green('SUCCESS: ') + 'No formatting errors found.');
-        }
-      }
+function logFiles(files) {
+  if (!isTravisBuild()) {
+    log(green('INFO: ') + 'Prettifying the following files:');
+    files.forEach(file => {
+      log(cyan(path.relative(rootDir, file)));
     });
+  }
+}
+
+/**
+ * Checks files for formatting (and optionally fixes them) with Prettier.
+ */
+async function prettify() {
+  maybeUpdatePackages();
+  let filesToCheck;
+  if (argv.files) {
+    filesToCheck = globby.sync(argv.files.split(','));
+    logFiles(filesToCheck);
+  } else if (argv.local_changes) {
+    filesToCheck = getFilesChanged(prettifyGlobs);
+    if (filesToCheck.length == 0) {
+      log(green('INFO: ') + 'No prettifiable files in this PR');
+      return;
+    } else {
+      logFiles(filesToCheck);
+    }
+  } else {
+    filesToCheck = globby.sync(prettifyGlobs);
+  }
+  runPrettify(filesToCheck);
+}
+
+/**
+ * Runs prettier on the given list of files. Returns the cumulative result to
+ * the `gulp` process via  process.exitCode so all files can be checked / fixed.
+ *
+ * @param {!Array<string>} filesToCheck
+ */
+function runPrettify(filesToCheck) {
+  if (!isTravisBuild()) {
+    log(green('Starting checks...'));
+  }
+  filesToCheck.forEach(file => {
+    checkFile(path.relative(rootDir, file));
+  });
+  if (process.exitCode == 1) {
+    logOnSameLine(red('ERROR: ') + 'Found errors in one or more files.');
+    if (!argv.fix) {
+      log(
+        yellow('NOTE 1:'),
+        'You may be able to automatically fix some errors by running',
+        cyan('gulp prettify --local_changes --fix'),
+        'from your local branch.'
+      );
+      log(
+        yellow('NOTE 2:'),
+        'Since this is a destructive operation (that edits your files',
+        'in-place), make sure you commit before running the command.'
+      );
+    }
+  } else {
+    if (!isTravisBuild()) {
+      logOnSameLine(green('SUCCESS: ') + 'No formatting errors found.');
+    }
+  }
 }
 
 /**
@@ -139,5 +171,6 @@ prettify.description =
   'Checks several non-JS files in the repo for formatting using prettier';
 prettify.flags = {
   'files': '  Checks only the specified files',
+  'local_changes': '  Checks just the files changed in the local branch',
   'fix': '  Fixes formatting errors',
 };
diff --git a/contributing/TESTING.md b/contributing/TESTING.md
@@ -56,11 +56,15 @@ Command                                                                 | Descri
 `gulp dist --noextensions`                                              | Builds production binaries without building any extensions.
 `gulp dist --fortesting`                                                | Builds production binaries for local testing. (Allows use cases like ads, tweets, etc. to work with minified sources. Overrides `TESTING_HOST` if specified. Uses the production `AMP_CONFIG` by default.)
 `gulp dist --fortesting --config=<config>`                              | Builds production binaries for local testing, with the specified `AMP_CONFIG`. `config` can be `prod` or `canary`. (Defaults to `prod`.)
-`gulp lint`                                                             | Validates against the ESLint linter.
+`gulp lint`                                                             | Validates JS files against the ESLint linter.
 `gulp lint --watch`                                                     | Watches for changes in files, and validates against the ESLint linter.
 `gulp lint --fix`                                                       | Fixes simple lint warnings/errors automatically.
 `gulp lint --files=<files-path-glob>`                                   | Lints just the files provided. Can be used with `--fix`.
 `gulp lint --local_changes`                                             | Lints just the files changed in the local branch. Can be used with `--fix`.
+`gulp prettify`                                                         | Validates non-JS files using Prettier.
+`gulp prettify --fix`                                                   | Fixes simple formatting errors automatically.
+`gulp prettify --files=<files-path-glob>`                               | Checks just the files provided. Can be used with `--fix`.
+`gulp prettify --local_changes`                                         | Checks just the files changed in the local branch. Can be used with `--fix`.
 `gulp build`                                                            | Builds the AMP library.
 `gulp build --extensions=amp-foo,amp-bar`                               | Builds the AMP library, with only the listed extensions.
 `gulp build --extensions=minimal_set`                                   | Builds the AMP library, with only the extensions needed to load `article.amp.html`.

diff --git a/package.json b/package.json
@@ -90,6 +90,7 @@
     "formidable": "1.2.1",
     "fs-extra": "8.1.0",
     "fuse.js": "3.4.5",
+    "globby": "10.0.1",
     "globs-to-files": "1.0.0",
     "google-closure-compiler": "20190929.0.0",
     "gulp": "4.0.2",
@@ -108,7 +109,6 @@
     "gulp-regexp-sourcemaps": "1.0.1",
     "gulp-rename": "1.4.0",
     "gulp-sourcemaps": "2.6.5",
-    "gulp-tap": "2.0.0",
     "gulp-watch": "5.0.1",
     "gzip-size": "5.1.1",
     "html-minifier": "4.0.0",

diff --git a/yarn.lock b/yarn.lock
@@ -5975,7 +5975,7 @@ globals@^9.18.0:
   resolved "https://registry.yarnpkg.com/globals/-/globals-9.18.0.tgz#aa3896b3e69b487f17e31ed2143d69a8e30c2d8a"
   integrity sha512-S0nG3CLEQiY/ILxqtztTWH/3iRRdyBLw6KMDxnKMchrtbj2OFmehVh0WUCfW3DUrIgx/qFrJPICrq4Z4sTR9UQ==
 
-globby@^10.0.1:
+globby@10.0.1, globby@^10.0.1:
   version "10.0.1"
   resolved "https://registry.yarnpkg.com/globby/-/globby-10.0.1.tgz#4782c34cb75dd683351335c5829cc3420e606b22"
   integrity sha512-sSs4inE1FB2YQiymcmTv6NWENryABjUNPeWhOvmn4SjtKybglsyPZxFB3U1/+L1bYi0rNZDqCLlHyLYDl1Pq5A==
@@ -6292,13 +6292,6 @@ gulp-sourcemaps@2.6.5:
     strip-bom-string "1.X"
     through2 "2.X"
 
-gulp-tap@2.0.0:
-  version "2.0.0"
-  resolved "https://registry.yarnpkg.com/gulp-tap/-/gulp-tap-2.0.0.tgz#6f66b79870dcbfc364cf4ebe0735b6008473200f"
-  integrity sha512-U5/v1bTozx672QHzrvzPe6fPl2io7Wqyrx2y30AG53eMU/idH4BrY/b2yikOkdyhjDqGgPoMUMnpBg9e9LK8Nw==
-  dependencies:
-    through2 "^3.0.1"
-
 gulp-util@~2.2.14:
   version "2.2.20"
   resolved "https://registry.yarnpkg.com/gulp-util/-/gulp-util-2.2.20.tgz#d7146e5728910bd8f047a6b0b1e549bc22dbd64c"