fix: consistently use a user-provided logger #176
Conversation
Codecov Report
@@ Coverage Diff @@
## main #176 +/- ##
=======================================
- Coverage 90.5% 90.5% -0.1%
=======================================
Files 7 7
Lines 722 721 -1
Branches 154 153 -1
=======================================
- Hits 654 653 -1
Misses 43 43
Partials 25 25
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
JonathanPlasse
force-pushed
the
main
branch
2 times, most recently
from
811ea99 to
21cbfea
Compare
There was a problem hiding this comment.
Sorry for the long delay. I must have missed this PR to begin with. Thanks for the ping, @JonathanPlasse. Let me have a look! :)
This looks good to me. 👍 Well done! I only have one gripe regarding default arguments. See the review for details.
With that small change, I'm ready to merge the code.
~Frederik
asyncio_mqtt/client.py
Outdated
| @@ -233,7 +233,7 @@ def __init__( # noqa: C901 | |||
| *, | |||
| username: str | None = None, | |||
| password: str | None = None, | |||
| logger: logging.Logger | None = None, | |||
| logger: logging.Logger = MQTT_LOGGER, | |||
There was a problem hiding this comment.
Prefer to use None for default values. It makes it easier to extend Client (e.g., through inheritance).
In turn, assign the actual default value inside the function itself. E.g.:
if logger is None:
logger = MQTT_LOGGER
self._logger = logger
self._client.enable_logger(logger)
Client initializer currently accepts a logger as an optional argument. That logger is passed to the underlying paho.mqtt library and then used for logging there. However, there also exists MQTT_LOGGER, which is used internally by asyncio-mqtt. Without looking at the source code it is easy to forget that in order to enable logging both in paho.mqtt and in asyncio-mqtt it is not enough to pass a logger as an argument to Client initializer. This commit makes asyncio-mqtt use a user-provided logger instead of MQTT_LOGGER for internal logging, so that if user decides to pass a logger to Client constructor, that logger will be used both in paho.mqtt and asyncio-mqtt.
|
LGTM. 👍 Thank you for your contribution to asyncio-mqtt. :) I apologize for the unnecessarily long PR review time on my part. I completely missed this PR (must have cleared my inbox a bit too fast 😅 ). |
|
That's not a problem at all. Thank you for the project :) |
Client initializer currently accepts a logger as an optional argument. That logger is passed to the underlying paho.mqtt library and then used for logging there. However, there also exists MQTT_LOGGER, which is used internally by asyncio-mqtt. Without looking at the source code it is easy to forget that in order to enable logging both in paho.mqtt and in asyncio-mqtt it is not enough to pass a logger as an argument to Client initializer.
This PR makes asyncio-mqtt use a user-provided logger instead of MQTT_LOGGER for internal logging, so that if user decides to pass a logger to Client constructor, that logger will be used both in paho.mqtt and asyncio-mqtt.