Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🏗 During PR checks, lint only the files in the PR, and flag errors instead of warnings #15256

Merged
merged 5 commits into from
May 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
"amphtml-internal/resolve-inside-promise-resolver": 1,
"amphtml-internal/todo-format": 0,
"amphtml-internal/unused-private-field": 1,
"amphtml-internal/vsync": 0,
"amphtml-internal/vsync": 1,
"array-bracket-spacing": [2, "never"],
"arrow-parens": [2, "as-needed"],
"arrow-spacing": 2,
Expand Down Expand Up @@ -123,6 +123,13 @@
"space-in-parens": 2,
"space-infix-ops": 2,
"space-unary-ops": [1, { "words": true, "nonwords": false }],
"valid-jsdoc": [1, {
"prefer": {"return": "return"},
"requireParamDescription": false,
"requireReturn": false,
"requireReturnType": true,
"requireReturnDescription": false
}],
"wrap-iife": [2, "any"]
}
}
14 changes: 14 additions & 0 deletions .eslintrc-strict
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
{
"rules": {
"amphtml-internal/resolve-inside-promise-resolver": 2,
"amphtml-internal/unused-private-field": 2,
"amphtml-internal/vsync": 2,
"valid-jsdoc": [2, {
"prefer": {"return": "return"},
"requireParamDescription": false,
"requireReturn": false,
"requireReturnType": true,
"requireReturnDescription": false
}]
}
}
35 changes: 31 additions & 4 deletions build-system/tasks/lint.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ const argv = require('minimist')(process.argv.slice(2));
const colors = require('ansi-colors');
const config = require('../config');
const eslint = require('gulp-eslint');
const getStdout = require('../exec').getStdout;
const gulp = require('gulp-help')(require('gulp'));
const gulpIf = require('gulp-if');
const lazypipe = require('lazypipe');
const log = require('fancy-log');
const path = require('path');
const watch = require('gulp-watch');

const isWatching = (argv.watch || argv.w) || false;
Expand Down Expand Up @@ -80,7 +82,8 @@ function logOnSameLine(message) {
function runLinter(path, stream, options) {
if (!process.env.TRAVIS) {
log(colors.green('Starting linter...'));
} else {
}
if (process.env.TRAVIS_EVENT_TYPE == 'push') {
// TODO(jridgewell, #14761): Remove log folding after #14761 is fixed.
log(colors.bold(colors.yellow('Lint results: ')) + 'Expand this section');
console./* OK*/log('travis_fold:start:lint_results\n');
Expand All @@ -97,7 +100,7 @@ function runLinter(path, stream, options) {
}))
.pipe(eslint.results(function(results) {
// TODO(jridgewell, #14761): Remove log folding after #14761 is fixed.
if (process.env.TRAVIS) {
if (process.env.TRAVIS_EVENT_TYPE == 'push') {
console./* OK*/log('travis_fold:end:lint_results');
}
if (results.errorCount == 0 && results.warningCount == 0) {
Expand All @@ -124,6 +127,19 @@ function runLinter(path, stream, options) {
.pipe(eslint.failAfterError());
}

/**
* Extracts the list of JS files in this PR from the commit log.
*
* @return {!Array<string>}
*/
function getLintFiles() {
const filesInPr =
getStdout('git diff --name-only master...HEAD').trim().split('\n');
return filesInPr.filter(function(file) {
return path.extname(file) == '.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a future change, do you mind adding '.ts' to this list or making a utility method the build process uses for filtering files based on extension?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Assuming it makes sense to run eslint checks on .ts files, we should definitely include them in gulp lint. Will test this out via a separate PR (there are other places that need to change too, so it's kinda out of scope for this PR.)

});
}

/**
* Run the eslinter on the src javascript and log the output
* @return {!Stream} Readable stream
Expand All @@ -132,8 +148,19 @@ function lint() {
if (argv.fix) {
options.fix = true;
}
if (argv.files) {
config.lintGlobs[config.lintGlobs.indexOf('**/*.js')] = argv.files;
if (argv.files ||
process.env.TRAVIS_PULL_REQUEST ||
process.env.LOCAL_PR_CHECK) {
if (argv.files) {
config.lintGlobs =
config.lintGlobs.filter(e => e !== '**/*.js').concat(argv.files);
} else {
config.lintGlobs =
config.lintGlobs.filter(e => e !== '**/*.js').concat(getLintFiles());
}
// TODO(#14761, #15255): Remove these overrides and make the rules errors by
// default in .eslintrc after all code is fixed.
options['configFile'] = '.eslintrc-strict';
}
const stream = initializeStream(config.lintGlobs, {});
return runLinter('.', stream, options);
Expand Down
3 changes: 1 addition & 2 deletions src/.eslintrc
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
{
"rules": {
"amphtml-internal/no-global": 2,
"amphtml-internal/vsync": 1
"amphtml-internal/no-global": 2
}
}