set command mishandles bool values

[pabigot]

When --set false reaches

cli/lib/control.js

Line 69 in 1cb3bfb

if (!Number.isNaN(options.set)) {

Number.isNaN("false") evaluates to false, so the conditional passes and the option set property becomes the result of Number.parseInt("false", 10), which is NaN, which has no effect when the device receives it.

A fix is to check for the acceptable string values first, then fall back to Number.parseInt.

[pabigot]

Also that code should be using isNaN, not Number.isNaN which does something different.

[codetheweb]

I believe what we should be doing is Number.isNaN(Number.parseInt('false', 10)). That would evaluate correctly.

If that sounds good, feel free to make a PR. Otherwise I can push a fix at some point.

[pabigot]

No, that's not right either, unless I misunderstand. Number.parseInt('12ac', 10) would return 12 so the wrong value would be passed without any warning.

Using isNaN instead of Number.isNaN would resolve that problem, but would still do the wrong thing for something like 1000 which could be a decimal number, or could be a 4-character hex string. OTOH in that case at least it's clear there's ambiguity, so one can say --raw-value should have been used to specify that the value should remain a string.

The two solutions that work are the original !isNaN(options.set) and !Number.isNaN(+options.set), but both are rejected by some style checker thing on commit.

 lib/control.js:1
 ✖  69:21  use Number(options.set) instead.              no-implicit-coercion   // this for !Number.isNaN(+options.set)
 ✖  69:8  Prefer Number.isNaN() over isNaN().  unicorn/prefer-number-properties  // this for isNaN(options.set)

#77 is the best I can do given the style checker complaints.