Skip to content

Commit

Permalink
feat: make lint check opt-in
Browse files Browse the repository at this point in the history
Disable lint check by default and make it opt-in, so it doesn't impact
the workflow for collaborators, while providing a way for early birds to
try it out so we can tweak it until it works great for everyone (at
which point we can re-enable it by default).

Ref: nodejs/node-core-utils#484
  • Loading branch information
Alicia Lopez committed Aug 20, 2020
1 parent 58face5 commit cf179f4
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 10 deletions.
11 changes: 8 additions & 3 deletions components/git/land.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ const landOptions = {
default: false,
type: 'boolean'
},
lint: {
describe: 'Run linter while landing commits',
default: false,
type: 'boolean'
},
autorebase: {
describe: 'Automatically rebase branches with multiple commits',
default: false,
Expand Down Expand Up @@ -99,8 +104,8 @@ function handler(argv) {

const provided = [];
for (const type of Object.keys(landOptions)) {
// --yes and --skipRefs are not actions.
if (type === 'yes' || type === 'skipRefs') continue;
// Those are not actions.
if (['yes', 'skipRefs', 'lint'].includes(type)) continue;
if (argv[type]) {
provided.push(type);
}
Expand Down Expand Up @@ -171,7 +176,7 @@ async function main(state, argv, cli, req, dir) {
return;
}
session = new LandingSession(cli, req, dir, argv.prid, argv.backport,
argv.autorebase);
argv.lint, argv.autorebase);
const metadata = await getMetadata(session.argv, argv.skipRefs, cli);
if (argv.backport) {
const split = metadata.metadata.split('\n')[0];
Expand Down
26 changes: 19 additions & 7 deletions lib/landing_session.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,25 @@ const { shortSha } = require('./utils');

const isWindows = process.platform === 'win32';

const LINT_RESULTS = {
SKIPPED: 'skipped',
FAILED: 'failed',
SUCCESS: 'success'
};

class LandingSession extends Session {
constructor(cli, req, dir, prid, backport, autorebase) {
constructor(cli, req, dir, prid, backport, lint, autorebase) {
super(cli, dir, prid);
this.req = req;
this.backport = backport;
this.lint = lint;
this.autorebase = autorebase;
}

get argv() {
const args = super.argv;
args.backport = this.backport;
args.lint = this.lint;
args.autorebase = this.autorebase;
return args;
}
Expand Down Expand Up @@ -147,14 +155,18 @@ class LandingSession extends Session {
async validateLint() {
// The linter is currently only run on non-Windows platforms.
if (os.platform() === 'win32') {
return true;
return LINT_RESULTS.SKIPPED;
}

if (!this.lint) {
return LINT_RESULTS.SKIPPED;
}

try {
await runAsync('make', ['lint']);
return true;
return LINT_RESULTS.SUCCESS;
} catch {
return false;
return LINT_RESULTS.FAILED;
}
}

Expand Down Expand Up @@ -229,13 +241,13 @@ class LandingSession extends Session {
const patch = await this.downloadAndPatch();

const cleanLint = await this.validateLint();
if (!cleanLint) {
if (cleanLint === LINT_RESULTS.FAILED) {
const tryFixLint = await cli.prompt(
'Lint failed - try fixing with \'make lint-js-fix\'?');
if (tryFixLint) {
await runAsync('make', ['lint-js-fix']);
const fixed = await this.validateLint();
if (!fixed) {
if (fixed === LINT_RESULTS.FAILED) {
cli.warn('Patch still contains lint errors. ' +
'Please fix manually before proceeding');
}
Expand All @@ -253,7 +265,7 @@ class LandingSession extends Session {
'`git node land --continue`.');
process.exit(1);
}
} else {
} else if (cleanLint === LINT_RESULTS.SUCCESS) {
cli.ok('Lint passed cleanly');
}

Expand Down

0 comments on commit cf179f4

Please sign in to comment.