UniversalDevicesInc / polyglot-v2-python-interface

Python Interface for NodeServers to Polyglot v2
MIT License
7 stars 4 forks source link

Check that message is a string in LoggerWriter to avoid stack overflow #12

Closed jimboca closed 5 years ago

jimboca commented 5 years ago

@Einstein42 you good with this change? It avoids the stack overflow issue I mentioned on slack.

firstone commented 5 years ago

I don't think it's very safe:

sys.version_info[0] 2 type('') <type 'str'>

I think safer way to check for type would be

if type(xxx) == type('')

or something like that.

jimboca commented 5 years ago

Yes, but that has issues with unicode in python 2 https://stackoverflow.com/questions/4843173/how-to-check-if-type-of-a-variable-is-string

firstone commented 5 years ago

I hate branching on version in code. Do you know what types it's breaking on? It is unicode? bytes? I still think it's better to do more explicit:

if type(xxx) == type('') or type(xxx) == type(u'') or type(xxx) == type(b''):

But if you don't feel that way, keep as-is. As long as it works.

jimboca commented 5 years ago

I hate branching on version in code. Do you know what types it's breaking on? It is unicode? bytes? I still think it's better to do more explicit:

if type(xxx) == type('') or type(xxx) == type(u'') or type(xxx) == type(b''):

But if you don't feel that way, keep as-is. As long as it works.

I really don't have a big opinion either way, so will let @Einstein42 make the call...

jimboca commented 5 years ago

@Einstein42 Any opinion? I have tested both and they work with python 3.

jimboca commented 5 years ago

@Einstein42 @exking @firstone Also added fix for SSLError discussed in the ecobee thread can you let me know your opinion on both these changes?

exking commented 5 years ago

@jimboca thanks, let's see how it goes :)