paho.mqtt.python icon indicating copy to clipboard operation
paho.mqtt.python copied to clipboard

MQTTv5. on_publish. Added reasonCode, properties arguments

Open trezorg opened this issue 3 years ago • 1 comments

trezorg avatar Apr 19 '22 18:04 trezorg

💯 Please add this in! This is very necessary

MikeDombo avatar May 25 '22 22:05 MikeDombo

Thank you for this great contribution! I stumbled into the same issue. With the help of your PR, I can now check the reasoncode in the on_publish-callback. It is the only way to find out if my published message has been rejected due to access control.

Next, we should also modify the return code of the publish function itself. Currently it is always Success - in contrast to the mqtt lib for nodejs, for example. (I described this in more detail in issue #715)

call-me-matt avatar Apr 27 '23 15:04 call-me-matt

It would be fantastic to get this merged in, we recently came across a bug related to this and have no workaround until this is merged in.

stonek4 avatar Jul 26 '23 17:07 stonek4

This will cause a breaking change. Any user that have a working client with MQTTv5 (but don't need reasonCode or properties) will have their client broken.

My two idea to avoid this is:

  • if doable reliably detect the number of argument accepted by on_publish and adapt argument send accordingly.
  • If the detection isn't doable or reliable enough (I think to *args or **kwargs cases), we might make it an option. But I think the option should not be tied to protocol used.

Also could you add a "Signed-Off" to your commit (option "-s" to git commit CLI). Eclipse ask us to verify its presence, which serve as confirmation that you agree with the ECA.

PierreF avatar Dec 20 '23 12:12 PierreF

I've just seen that we already had something similar to you change with the on_connect callback. But I still think we should avoid breaking.

After a second though I think a explicit option to decide which callback API to use is better than auto-detection.

PierreF avatar Dec 22 '23 16:12 PierreF

Thank for your contribution. Closing this PR because #802 fix the same issue as this PR.

PierreF avatar Jan 21 '24 10:01 PierreF