From 462f95e126ad9ea02d01efafe79e23904e66545b Mon Sep 17 00:00:00 2001 From: Noah Brenner Date: Wed, 18 Jul 2018 20:48:14 -0700 Subject: [PATCH 1/2] Fix CLI's `--space` option parsing 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. --- main.js | 14 ++++++++++++++ test/fixtures/space/one-space.js | 3 +++ test/fixtures/space/two-spaces.js | 3 +++ test/main.js | 27 +++++++++++++++++++++++++++ 4 files changed, 47 insertions(+) create mode 100644 test/fixtures/space/one-space.js create mode 100644 test/fixtures/space/two-spaces.js diff --git a/main.js b/main.js index d3b5e244..ae572db1 100755 --- a/main.js +++ b/main.js @@ -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; diff --git a/test/fixtures/space/one-space.js b/test/fixtures/space/one-space.js new file mode 100644 index 00000000..0b1b5e7d --- /dev/null +++ b/test/fixtures/space/one-space.js @@ -0,0 +1,3 @@ +console.log([ + 1 +]); diff --git a/test/fixtures/space/two-spaces.js b/test/fixtures/space/two-spaces.js new file mode 100644 index 00000000..baa2c240 --- /dev/null +++ b/test/fixtures/space/two-spaces.js @@ -0,0 +1,3 @@ +console.log([ + 1 +]); diff --git a/test/main.js b/test/main.js index 07b6a5a1..568df16b 100644 --- a/test/main.js +++ b/test/main.js @@ -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); +}); From e5cb28d6d6f318cced4046a5da0155e672945dd0 Mon Sep 17 00:00:00 2001 From: Noah Brenner Date: Wed, 25 Jul 2018 21:40:12 -0700 Subject: [PATCH 2/2] Support 'true' and 'false' values for `--space` --- main.js | 16 ++++++++++------ test/main.js | 8 ++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/main.js b/main.js index ae572db1..cf8657ed 100755 --- a/main.js +++ b/main.js @@ -115,15 +115,19 @@ updateNotifier({pkg: cli.pkg}).notify(); const {input, flags: opts} = cli; // Make data types for `opts.space` match those of the API +// Check for string type because `xo --no-space` sets `opts.space` to `false` 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') { + } else if (opts.space === 'true') { + opts.space = true; + } else if (opts.space === 'false') { + opts.space = false; + } else { + if (opts.space !== '') { + // Assume `opts.space` was set to a filename when run as `xo --space file.js` + input.push(opts.space); + } opts.space = true; } } diff --git a/test/main.js b/test/main.js index 568df16b..b55ab082 100644 --- a/test/main.js +++ b/test/main.js @@ -142,3 +142,11 @@ test('space option as boolean with filename', async t => { // The default space value of 2 was expected t.is(reports[0].errorCount, 0); }); + +test('space option with boolean strings', async t => { + const cwd = path.join(__dirname, 'fixtures/space'); + const trueResult = await t.throws(main(['--space=true'], {cwd})); + const falseResult = await t.throws(main(['--space=false'], {cwd})); + t.true(trueResult.stdout.includes('Expected indentation of 2 spaces')); + t.true(falseResult.stdout.includes('Expected indentation of 1 tab')); +});