alexylem / jarvis-api

Plugin to allow controling Jarvis remotely using RestAPI
10 stars 7 forks source link

Wrong boolean handling for mute and verbose #15

Closed Oliv4945 closed 7 years ago

Oliv4945 commented 7 years ago

Hi,

Thanks to @NikoS34 here I discovered how you handle verbose and mute variables: the current code only detects if the variable is set, not its boolean value

I would propose the following change, but it breaks compatibility with GET requests as True is recognized as a string. Do you have any idea ? A workaround would be to test type (String or boolean) but maybe there is a cleaner code.

Oliv'

    mute = data.get("mute", False)
    jarvis.mute_mode = mute == True
    verbose = data.get("verbose", False)
    jarvis.verbose = verbose == True
alexylem commented 7 years ago

@Oliv4945 you are right there is a bug, and not a small one 😄

I noticed that in the browser (with GET parameters) it's not working:

http://192.168.1.20:8080/?order=bonjour&mute=false&verbose=false
# will be mute and verbose

Need to fix this.

However, Jarvis-UI sends true or false in the parameters and it works. I think this is because the data is sent with POST and in the python code evaluates that boolean with the parenthesis (with POST it is apparently not a string but a real boolean):

jarvis.mute_mode = ("mute" in data) and (data ["mute"])
jarvis.verbose = ("verbose" in data) and (data ["verbose"])

As a workaround for your App, can you make the requests with POST ? If not, is "true" and "false" as values for GET ok for you?

Oliv4945 commented 7 years ago

think this is because the data is sent with POST and in the python code evaluates that boolean with the parenthesis (with POST it is apparently not a string but a real boolean):

With a GET request you can only send strings. With POST request, Python parse data as a JSON object, so a boolean: {"key": "testkey", "mute": true, "verbose":false}

As a workaround for your App, can you make the requests with POST

Already published here, I am not sending "mute" if not required. By the way I already use a POST request but I was sending a string instead of a Boolean :$

alexylem commented 7 years ago

Okay. I will hardcode a check if string and not equal "false".

Oliv4945 commented 7 years ago

So a way to go might be (not tested)

mute = data.get("mute", False)
if isinstance(mute, str)
    if mute.lower() == 'true':
        mute = True
    else:
        mute = False
jarvis.mute_mode = mute
alexylem commented 7 years ago

I did more or less the same (saw your message after commit):

if "mute" in data:
        mute=data["mute"]
        jarvis.mute_mode=mute if isinstance(mute, bool) else (mute != "false")
Oliv4945 commented 7 years ago

Ok, you should put lower() in case somebody send the string False

if "mute" in data:
        mute=data["mute"]
        jarvis.mute_mode=mute if isinstance(mute, bool) else (mute.lower() != "false")

Just by curiosity, why don't you put mute == 'true' instead ?

alexylem commented 7 years ago

@Oliv4945 yes I could but the API docs says "true", and I always prefer to keep the code as simple as possible. But I could change here no big deal if it's easier for you. Now why != false and not == true I think it's more of a philosophical question 😄 Maybe coding influence where everything that is not really "false" is true.

Oliv4945 commented 7 years ago

@Oliv4945 yes I could but the API docs says "true", and I always prefer to keep the code as simple as possible. But I could change here no big deal if it's easier for you.

It does not change anything for me but be prepared to have an issue one day or another, always take the place of a fool and see what can go wrong :p

Maybe coding influence where everything that is not really "false" is true.

Ok, funny thing because for me what is not explicitly true is false :-)

alexylem commented 7 years ago

You won 😄