search

tfriedel / python-lightify

Python module for Osram lightify. This is a work in progress.
Apache License 2.0
18 stars 9 forks source link

add optional loghandler and method to change log-level #15

Closed rbdubz3 closed 5 years ago

rbdubz3 commented 5 years ago

minor feature request to allow an external log handler and change in log level during debugging

tfriedel commented 5 years ago

thanks for the PR, will check it out soon. @OleksandrBerchenko or @rbdubz3 : can you check if this works with the logger of HA? I just tested the version of lightify before this PR and noticed the logging is not working as it's supposed to. HA has a logger component where you can set log levels for different components or a general log leveI. I see this one warning out of lightify, with the unknown device id lamp. However when I set the HA logger to "debug", I didn't see all the debug messages.

rbdubz3 commented 5 years ago

@tfriedel - yeah I went thru something similar with my Indigo plugin, which is the reason for the handler.. I use this handler to write to the Indigo log, and the plugin allows the administrator to toggle debug on/off - so the 'set_level' method allows me to additionally set the log level for Lightify library to match that of the plugin

I don't have HA at all so I cannot test this one

OleksandrBerchenko commented 5 years ago

@tfriedel Ok, I will test it and integrate with HA if it works as expected.

@rbdubz3 Please add a missing docstring (see my comment) and remove the code related to level_str. You have applied my code suggestions, but I can provide them only for single line changes (it's a beta feature on GitHub and it has some limitations).

Does it make sense to pass a default log level into __init__ instead of hardcoded INFO level? It seems to me that in most cases you already decided about the log level when you create a Lightify object. A situation when you need to change the log level dynamically is probably an edge case.

Thanks!

rbdubz3 commented 5 years ago

@OleksandrBerchenko - let me add the suggested fixes and issue a new PR.

You are probably right with passing the log level to init . I'll let you guys make that change if you want since the constructor change will probably mess up your tests

rbdubz3 commented 5 years ago

gonna open a new PR with all the requested changes - including change to init for the log level