search

eclipse / paho.mqtt.python

paho.mqtt.python
Other
2.17k stars 723 forks source link

MQTT v5 callback parameters for `on_subscribe` and `on_unsubscribe` callback handlers swapped #687

Closed jbouwh closed 1 month ago

jbouwh commented 1 year ago

The MQTT v5 callback parameters for reasoncodes and properties are swapped for on_subscribe() and on_unsubscribe(). This is very confusing and makes it harder to reuse the callback handler.

on_subscribe(self, self._userdata, mid, reasoncodes, properties)

https://github.com/eclipse/paho.mqtt.python/blob/9782ab81fe7ee3a05e74c7f3e1d03d5611ea4be4/src/paho/mqtt/client.py#L3258-L3259

on_unsubscribe(self, self._userdata, mid, properties, reasoncodes)

https://github.com/eclipse/paho.mqtt.python/blob/9782ab81fe7ee3a05e74c7f3e1d03d5611ea4be4/src/paho/mqtt/client.py#L3452-L3453

MattBrittan commented 8 months ago

This is annoying but resolving it would require a breaking change; as such I'm going to tag it as an enhancement.

jbouwh commented 8 months ago

True, may be we should deprecation the old form and come with a new API that will replace the old one, so they can run some time together.

MattBrittan commented 1 month ago

Closing this as the V2 API made the calls consistent.

jbouwh commented 1 month ago

One note though. The type stubs seem not updated yet. So mypy will complain about incorrect typing or the absence of CallbackAPIVersion, this should be fixed too before we can upgrade to 2.1.0.

MattBrittan commented 1 month ago

@jbouwh (working way outside my knowledge level here :-) ) - I believe the type info is all there:

CallbackOnSubscribe_v1_mqtt3 = Callable[["Client", Any, int, Tuple[int, ...]], None]
CallbackOnSubscribe_v1_mqtt5 = Callable[["Client", Any, int, List[ReasonCode], Properties], None]
CallbackOnSubscribe_v1 = Union[CallbackOnSubscribe_v1_mqtt3, CallbackOnSubscribe_v1_mqtt5]
CallbackOnSubscribe_v2 = Callable[["Client", Any, int, List[ReasonCode], Union[Properties, None]], None]
CallbackOnSubscribe = Union[CallbackOnSubscribe_v1, CallbackOnSubscribe_v2]

but it's pretty complicated due to the multiple API versions. Might be worth raising a seperate issue with full details (but I don't know if this may be a limitation with the tool, intellij seems to pickup the type info OK and lists a multitude of possibilities).

jbouwh commented 1 month ago

@jbouwh (working way outside my knowledge level here :-) ) - I believe the type info is all there:

CallbackOnSubscribe_v1_mqtt3 = Callable[["Client", Any, int, Tuple[int, ...]], None]
CallbackOnSubscribe_v1_mqtt5 = Callable[["Client", Any, int, List[ReasonCode], Properties], None]
CallbackOnSubscribe_v1 = Union[CallbackOnSubscribe_v1_mqtt3, CallbackOnSubscribe_v1_mqtt5]
CallbackOnSubscribe_v2 = Callable[["Client", Any, int, List[ReasonCode], Union[Properties, None]], None]
CallbackOnSubscribe = Union[CallbackOnSubscribe_v1, CallbackOnSubscribe_v2]

but it's pretty complicated due to the multiple API versions. Might be worth raising a seperate issue with full details (but I don't know if this may be a limitation with the tool, intellij seems to pickup the type info OK and lists a multitude of possibilities). Right, the code works btw, it is just the type stubs, imo multiple existing issues have the same root cause.