From 56bc0dec7c235238f6e0aa480b40aad2dbfda295 Mon Sep 17 00:00:00 2001
From: Noah Brenner <noah@noahbrenner.com>
Date: Wed, 18 Jul 2018 20:48:14 -0700
Subject: [PATCH] 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 925f2c04..2ab2e3d8 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);
+});