Skip to content

Commit

Permalink
🏗 Add a --local_changes mode to gulp prettify (ampproject#25093)
Browse files Browse the repository at this point in the history
  • Loading branch information
@rsimha
rsimha authored and Micajuine Ho committed Dec 27, 2019
1 parent 07f64f0 commit 582253f
Show file tree
Hide file tree
Showing 8 changed files with 108 additions and 66 deletions.
5 changes: 5 additions & 0 deletions build-system/common/default-pre-push
View file
Expand Up @@ -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.)")
Expand All @@ -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"
Expand Down
22 changes: 22 additions & 0 deletions build-system/common/utils.js
View file
Expand Up @@ -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) {
Expand All @@ -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,
};
21 changes: 3 additions & 18 deletions build-system/tasks/lint.js
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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();
Expand All @@ -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',
};
2 changes: 1 addition & 1 deletion build-system/tasks/pr-check.js
View file
Expand Up @@ -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');

Expand Down
107 changes: 70 additions & 37 deletions build-system/tasks/prettify.js
View file
Expand Up @@ -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));
Expand All @@ -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.');
}
}
}

/**
Expand Down Expand Up @@ -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',
};
6 changes: 5 additions & 1 deletion contributing/TESTING.md
View file
Expand Up @@ -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`.
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
9 changes: 1 addition & 8 deletions yarn.lock
View file
Expand Up @@ -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==
Expand Down Expand Up @@ -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"
Expand Down

0 comments on commit 582253f

Please sign in to comment.