diff --git a/build-system/common/default-pre-push b/build-system/common/default-pre-push index 263ff6a19e47..9d022c31ea8c 100755 --- 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 index c4d805a71d1c..bf9a4fe2d985 100644 --- 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} globs + * @return {!Array} + */ +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 index d87a83c1d358..de3f6340a888 100644 --- 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} - */ -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 index 4618b5ca2f54..2313f96e5e6e 100644 --- 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 index e8ece276a141..e8101c1fd7f7 100644 --- 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} 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} 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 index bc5ecf95b02c..cdd7c226d0f6 100644 --- 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=` | 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=` | 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=` | 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 index 51c41602e799..770b6c0d1fbe 100644 --- 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 index a94dc23affd0..a5eaab0dbe05 100644 --- 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"