TuyaAPI / cli

🔧 A CLI for Tuya devices
MIT License
261 stars 44 forks source link

set command mishandles bool values #75

Closed pabigot closed 4 years ago

pabigot commented 4 years ago

When --set false reaches https://github.com/TuyaAPI/cli/blob/1cb3bfb58f1f9817a50838f7ca674fe582f26e09/lib/control.js#L69 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 commented 4 years ago

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

codetheweb commented 4 years ago

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 commented 4 years ago

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.