Fix CLI's `--space` option parsing #342

noahbrenner · 2018-07-23T08:33:45Z

Overview

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 led to a few bugs.

Bugs fixed

Here are the results of several invocations before and after applying this PR:

`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 recognized positional 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.

Edge cases

These are some quirks that are still present after this PR. Some of them may actually be desired, depending on your viewpoint, others perhaps less so. Let me know if there are any that you would like me to address.

xo --space=false: 'false' is treated as a file name and 2-space indents are expected. It is similar for --space=true.
xo --space=3.0: '3.0' is treated as a file name and 2-space indents are expected. The implementation in this PR only recognizes numbers if they contain nothing but digits. This also applies to negative numbers (no - allowed).
The radix is always treated as 10, even with leading 0s.
xo --space=0: Tabs will be expected, since the value is falsey.
There is no check against Number.MAX_SAFE_INTEGER, so weird rounding and overflow can happen if someone wants the worst indent ever.
xo --space=4 --no-space: This results in an array of ['4', false], so space will be truthy, 2-space indents will be expected, and this array will be passed to the main API as if it were a positional argument.
Since non-number values of space are now assumed to be file names (or globs), it is possible to specify a filename before the end of the command line options: xo --space file.js --quiet another.js.

References

Some relevant lines elsewhere in the code base:

Whether to expect tabs or spaces is based on the truthiness of the space option, not just its presence.
Setting the number of spaces to something other than the default of 2 depends on the value being a number (not a string, for example).

Closes #339
Possibly relevant to #212 when specifying a file directly after --space

sindresorhus · 2018-07-23T09:05:44Z

main.js

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


Relevant meow issue: sindresorhus/meow#44

sindresorhus · 2018-07-23T09:06:15Z

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)) {


You can drop the +. The number will never be more than 8.

sindresorhus · 2018-07-23T09:06:38Z

test/main.js

+	const reports = JSON.parse(stdout);
+
+	// Only the specified file was checked (filename was not the value of `space`)
+	t.true(reports.length === 1);


Use t.is here and below.

noahbrenner · 2018-07-23T09:30:33Z

Sounds good, I've updated those now.

pvdlg · 2018-07-24T01:48:29Z

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') {


space is configured as type String here. So yarg-parser will always produce a String.
I tried every possibility I could think of with yargs-parser and always get either a String or no space property at all.

Therefore the test if (typeof opts.space === 'string') is unnecessary and should be replaced by if (typeof opts.space !== 'undefined').

The one exception is if you pass --no-space, then the value will be false. Is that an important case or would you still like it to just check against undefined?

Yes indeed to we can keep (typeof opts.space === 'string').

pvdlg · 2018-07-24T01:51:28Z

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)) {


/^\d$/ should be /^\d+$/ otherwise all number with multiple digit won't match (for example 10)

That's actually what I wrote initially and @sindresorhus asked me to remove the +, saying that the number would never be larger than 8 (see his second comment on this PR). What's the consensus on this?

I didn't see this comment. The problem if we drop the + is that xo --space 10 will be considered as trying to lint the file 10. I'm not sure this is what we want. @sindresorhus did you considered that case?

In addition if we really want to limit that value to 0-8 we should explicitly test it and report the error if it's not valid similarly to https://github.com/xojs/xo/blob/master/main.js#L136

Then I would favor making anything more than 8 an error. It makes no sense to do that and is most likely an error.

Do we want to throw that error here, though, or should that be in options-manager.js so that the limit applies to both the CLI and the API?

Should be in the API.

Just to confirm, I'm hearing that we should checking CLI input using /^\d+$/ (with the +) so that we don't treat higher numbers as filenames, then enforce the 8-space limit from the API.

Do you want that limit included in this PR, or should that be separate, since its a different scope?

pvdlg · 2018-07-24T01:54:17Z

main.js

+		input.push(opts.space);
+	}
+
+	if (typeof opts.space !== 'number') {


This test depends on the fact that a previous branch was executed or not, so it's quite confusing.
Like you have the test (typeof opts.space !== 'number') within the test (typeof opts.space === 'string').

I'm happy to adjust that. Here's my reasoning for doing it this way: in the non-number branch of the first if statement, I wouldn't want to unconditionally append to input since the value could be '', which wouldn't be useful as a file name. I could absolutely nest this test inside of the first one, though.

It makes sense now that you explain it, but by reading the code the fact you meant to unconditional append to input since the value could be '' is really not clear. It not really clear that the value can be '' unless you know all details of the parser by heart.
The code is locally valid, but most likely if we try to modify it in 6 months we'll never figure that out.

Good point! When I move that condition into the first block I can also add a comment to that effect.

pvdlg · 2018-07-24T02:42:00Z

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') {


See my other comments regarding this code.
In addition the cases where yargs-parser set space as 'true' or 'false' is not handled.
I think would be clearer and cover the 'true' or 'false' case:

if (typeof opts.space !== 'undefined') { if (opts.space === 'true') { opts.space = true; } else if (opts.space === 'false') { opts.space = false; } else if (/^\d+$/.test(opts.space)) { opts.space = parseInt(opts.space, 10); } if (opts.space) { input.push(opts.space); } opts.space = true; }

That's a great idea, handling those 'true' and 'false' cases! I'll get on that. I'd do a slightly different implementation, though, since that quick example would always set the value to true and would push numbers and true to input.

If we add the + to the regexp then the numbers wouldn't be parsed as the input.
Indeed the opts.space = true; is wrongly positioned. It should probably be something like that:

if (typeof opts.space !== 'undefined') { if (opts.space === 'true') { opts.space = true; } else if (opts.space === 'false') { opts.space = false; } else if (/^\d+$/.test(opts.space)) { opts.space = parseInt(opts.space, 10); } else { if (opts.space) { input.push(opts.space); } opts.space = true; } }

pvdlg · 2018-07-24T02:43:40Z

@sindresorhus that would also be a good idea to fix Travis and the unit test. See #341

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.

noahbrenner · 2018-07-26T05:02:46Z

I've just updated the PR.

Following up on our discussions above:

I kept (typeof opts.space === 'string') but added a comment to explain why the value could be a boolean in addition to undefined or a string.
I changed the RegEx back to /^\d+$/ to handle larger numbers (or ones with leading zeros).
- At this point, I haven't added a check to limit the option to a max of 8. Since that change relates to the main API and not just the CLI, I'm figuring that should be a separate change/PR. Let me know if you'd like me to either add that constraint to this PR or open a new one for that (or if you want to handle that yourselves).
I added support for --space=true and --space=false and refactored the if block as we discussed.
- I ended up testing for (opts.space !== '') before appending to input. Usually, I'd tend to just test (opts.space), but in this case it may be more helpful to communicate what value we're specifically avoiding adding to the input array.

noahbrenner · 2018-07-28T05:40:58Z

@sindresorhus Any other changes you'd like?

sindresorhus · 2018-07-28T14:43:41Z

@noahbrenner Looking good. Thanks for this awesome contribution :)

noahbrenner · 2018-07-29T21:33:42Z

Absolutely! And thanks to you two for working with me on it!

sindresorhus reviewed Jul 23, 2018

View reviewed changes

main.js Outdated

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

opts.space = true;

}

}

Copy link

Member

sindresorhus Jul 23, 2018

Choose a reason for hiding this comment

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

Relevant meow issue: sindresorhus/meow#44

sindresorhus reviewed Jul 23, 2018

View reviewed changes

noahbrenner force-pushed the space-parsing branch from d232772 to 56bc0de Compare July 23, 2018 09:28

sindresorhus requested a review from pvdlg July 23, 2018 09:36

pvdlg suggested changes Jul 24, 2018

View reviewed changes

noahbrenner added 2 commits July 25, 2018 20:41

Support 'true' and 'false' values for --space

e5cb28d

noahbrenner force-pushed the space-parsing branch from 56bc0de to e5cb28d Compare July 26, 2018 05:00

pvdlg approved these changes Jul 26, 2018

View reviewed changes

sindresorhus merged commit f76c901 into xojs:master Jul 28, 2018

noahbrenner deleted the space-parsing branch July 29, 2018 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CLI's `--space` option parsing #342

Fix CLI's `--space` option parsing #342

noahbrenner commented Jul 23, 2018

sindresorhus Jul 23, 2018

sindresorhus Jul 23, 2018

sindresorhus Jul 23, 2018

noahbrenner commented Jul 23, 2018

pvdlg Jul 24, 2018

noahbrenner Jul 24, 2018

pvdlg Jul 24, 2018

pvdlg Jul 24, 2018

noahbrenner Jul 24, 2018

pvdlg Jul 24, 2018

pvdlg Jul 24, 2018

sindresorhus Jul 24, 2018

noahbrenner Jul 24, 2018

sindresorhus Jul 24, 2018

noahbrenner Jul 24, 2018

pvdlg Jul 24, 2018

noahbrenner Jul 24, 2018

pvdlg Jul 24, 2018

noahbrenner Jul 24, 2018

pvdlg Jul 24, 2018

noahbrenner Jul 24, 2018

pvdlg Jul 24, 2018

pvdlg commented Jul 24, 2018

noahbrenner commented Jul 26, 2018 •

edited

Loading

noahbrenner commented Jul 28, 2018

sindresorhus commented Jul 28, 2018

noahbrenner commented Jul 29, 2018

Fix CLI's --space option parsing #342

Fix CLI's --space option parsing #342

Conversation

noahbrenner commented Jul 23, 2018

Overview

Bugs fixed

xo --space=4

Before fix:

Now:

xo --space file.js

Before fix:

Now:

xo --space

Before fix:

Now:

Edge cases

References

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noahbrenner commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdlg commented Jul 24, 2018

noahbrenner commented Jul 26, 2018 • edited Loading

noahbrenner commented Jul 28, 2018

sindresorhus commented Jul 28, 2018

noahbrenner commented Jul 29, 2018

Fix CLI's `--space` option parsing #342

Fix CLI's `--space` option parsing #342

`xo --space=4`

`xo --space file.js`

`xo --space`

noahbrenner commented Jul 26, 2018 •

edited

Loading