Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider converting ReasonCodes into readable strings in MqttCodeError #73

Closed
CptSpaceToaster opened this issue Aug 16, 2021 · 2 comments

Comments

@CptSpaceToaster
Copy link
Contributor

CptSpaceToaster commented Aug 16, 2021

Hi, new to the project, and loving where it's going.

I thought about opening a PR to start... but maybe some context first might make more sense. When debugging my app, I ran into a situation where a timeout would cause paho to stop sending all messages. I was catching and logging the errors, but missed the subtle difference between the MqttError and MqttCodeError types behind the scenes.

For reasons still somewhat unknown, the reason code was being set to an integer (not a mqtt.ReasonCode) and the subtle [Code: 4] added to the __str__() had slipped by. Once I took a deeper look at what was happening, I saw the extra information, as well as a helper method in paho's source code: https://github.com/eclipse/paho.mqtt.python/blob/master/src/paho/mqtt/client.py#L183

The simple complaint here, is that [Code: {}] is a small addition to an error message, easy to miss, and a not-very-idiomatic error number, when it could instead be an english error message in instances where the rc value is actually an integer.

using Paho's error_message handler or using their builtin methods, we could change the __str__() method in MqttCodeError to look something a little like this:

class MqttCodeError(MqttError):
    def __init__(self, rc: Union[int, mqtt.ReasonCodes], *args: Any):
        super().__init__(*args)
        self.rc = rc

    def __str__(self) -> str:
        if isinstance(self.rc, mqtt.ReasonCodes):
            return f"[code:{self.rc.value}] {str(self.rc)}"
        else:
            return f"[code:{self.rc}] {mqtt.error_string(self.rc)} {super().__str__()}"

But for such a simple change... I figured it'd be better to ask if there was a reason it was setup this way, or if I'm touching something that hasn't been considered in a while. Maybe my versions are worth considering? paho-mqtt-1.5.1 and asyncio_mqtt-0.9.1

If this sounds like a useful change, I'd be more than willing to open up a simple PR... otherwise I'd be interested in learning why the integer codes are preferred to Paho's internal error messages.

@frederikaalund
Copy link
Collaborator

Hi CptSpaceToaster, thanks for opening this issue. Let me have a look. :)

That's a really good idea. I'd love to have that as part of asyncio-mqtt. Feel free to open a pull request. 👍

Sorry about the lackluster MqttCodeError. It's certainly not a usability master piece. 😅

@CptSpaceToaster
Copy link
Contributor Author

Do you also want to see the additional {super().__str__()} call in there? In my use case, 100% of the time, that will end up saying Could not publish message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants