Skip to content

Commit

Permalink
Fix CLI's --space option parsing
Browse files Browse the repository at this point in the history
Ensure that the `space` value, if present, is either a number or a
boolean before passing it to the main API.  The space option is intended
to act as either a boolean or a number option, but it is implemented as
a string option out of necessity, which resulted in some bugs.

This commit fixes bugs demonstrated by the following invocations:

`xo --space=4`
Before fix:
 Expect 2-space indents.
 * The value of `space` is the *string* '4'.
 * The value is truthy, resulting in the space option being activated.
 * The value is not a number, resulting in the default of 2 being used.
Now: Expect 4-space indents.
 * The value of `space` is the *number* 4.

`xo --space file.js`
Before fix:
 Check all files in the CWD and expect 2-space indents.
 * The value of `space` is 'file.js'.
 * The value is truthy and not a number, so expect 2-space indents.
 * There are no positional arguments recognized, so check all files.
   - This is a subtle bug, since the desired file *is* usually checked,
     it's just not checked exclusively.
Now:
 Check only file.js and expect 2-space indents.

`xo --space`
Before fix:
 Expect tab indents.
 - The value of `space` is the empty string.
 - The value is falsey, so it is not used, instead defaulting to tabs.
Now:
 Expect 2-space indents.
  • Loading branch information
noahbrenner committed Jul 23, 2018
1 parent 404d81b commit 56bc0de
Show file tree
Hide file tree
Showing 4 changed files with 47 additions and 0 deletions.
14 changes: 14 additions & 0 deletions main.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,20 @@ updateNotifier({pkg: cli.pkg}).notify();

const {input, flags: opts} = cli;

// Make data types for `opts.space` match those of the API
if (typeof opts.space === 'string') {
if (/^\d$/.test(opts.space)) {
opts.space = parseInt(opts.space, 10);
} else if (opts.space) {
// Assume `opts.space` was set to a filename when run as `xo --space file.js`
input.push(opts.space);
}

if (typeof opts.space !== 'number') {
opts.space = true;
}
}

const log = report => {
const reporter = opts.reporter ? xo.getFormatter(opts.reporter) : formatterPretty;

Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/space/one-space.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
console.log([
1
]);
3 changes: 3 additions & 0 deletions test/fixtures/space/two-spaces.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
console.log([
1
]);
27 changes: 27 additions & 0 deletions test/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,30 @@ test('cli option takes precedence over config', async t => {
// i.e make sure absent cli flags are not parsed as `false`
await t.throws(main(['--stdin'], {input}));
});

test('space option with number value', async t => {
const cwd = path.join(__dirname, 'fixtures/space');
const {stdout} = await t.throws(main(['--space=4', 'one-space.js'], {cwd}));
t.true(stdout.includes('Expected indentation of 4 spaces'));
});

test('space option as boolean', async t => {
const cwd = path.join(__dirname, 'fixtures/space');
const {stdout} = await t.throws(main(['--space'], {cwd}));
t.true(stdout.includes('Expected indentation of 2 spaces'));
});

test('space option as boolean with filename', async t => {
const cwd = path.join(__dirname, 'fixtures/space');
const {stdout} = await main(['--reporter=json', '--space', 'two-spaces.js'], {
cwd,
reject: false
});
const reports = JSON.parse(stdout);

// Only the specified file was checked (filename was not the value of `space`)
t.is(reports.length, 1);

// The default space value of 2 was expected
t.is(reports[0].errorCount, 0);
});

0 comments on commit 56bc0de

Please sign in to comment.