| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
6 years later... I was researching before making the exact same PR. Having read all of the comments on #186, I agree that users of umqtt.robust are responsible for resubscribing, and this PR makes it easier to do so. To that end, my after_connect() is only called if the broker returns a clean session, so I have an extra if compared to the example above (line 6): from umqtt import robust
class MQTTClient(robust.MQTTClient):
def connect(self, clean_session=True):
res = super().connect(clean_session)
if not res:
self.after_connect()
return res
def after_connect():
#use for mqtt subscribe
return@pfalcon and @dpgeorge: FYA - no extra code, no broadening of the scope of the module, behaviour identical to the current umqtt.robust, just more easily extensible. |
Sorry, something went wrong.
|
@andrewleech, fancy reviewing / merging this PR? Do you see any pitfalls? |
Sorry, something went wrong.
|
I actually think it looks really strange to call self.connect when there is no actual self.connect function, it's confusing. I can guarantee new users coming to this module won't understand the need for this, at least not without examples / docs. I think if the end goal is to make it easier for users to make reconnect re-subscribe as needed, assuming this is a common need, scaffolding to do exactly that should be added instead. |
Sorry, something went wrong.
|
I appreciate the review, and still think the PR, as is, has merit. Addressing the general thrust of your critique... Strange and confusingAgreed, to the uninitiated, the reference to self.connect() may look strange and be confusing. That doesn't make it wrong however and, as you suggest, it could be explained in docs / examples which I'd be happy to add if you agree. The end goalDepending upon who you cite, the 'end goal' can differ quite a lot. Expanding robust's scope has been debated at length, particularly in #186, and I'm generally content with its present scope (my interpretation):
A common enough wish is that robust would provide automated re-subscription when a clean session is returned from connect() within reconnect(); the cost: code size (1 above). This can already be achieved through (2 above) so I agree with the argument not to change the module. OutcomesThis PR makes implementing re-subscription more compact: without it, the user has to override reconnect() thereby repeating the 8 lines of the same method in robust before adding code to re-subscribe - not very DRY. With this PR, the user has to call super().connect() at some point, but is otherwise free to focus on their application's logic. I think it bears repeating that this change doesn't increase code size and doesn't alter existing built-in behaviour. An elegant solution that makes it easier for users to leverage robust whilst building upon it.
This is a separate issue IMHO. It has merit and I think people would use it, but I think it would form part of a third umqtt module. I'd be happy to collaborate because that's effectively what I'm implementing - just think it should stay clear of robust. |
Sorry, something went wrong.
|
When you mention a possible third version, well to be honest I've never used this robust module because any time I want to use mqtt I go straight to https://github.com/peterhinch/micropython-mqtt and in particular his async version. Regardless, I appreciate the argument that this change here keeps it lean, and with an example and/or docs it has merit in terms of a cheap increase in flexibility. |
Sorry, something went wrong.
There was a problem hiding this comment.
This looks good to me, a very simple change that allows extensibility (albeit you have to know where to look).
Sorry, something went wrong.
This allow overriding of the `connect()` method by a subclass, eg:
from umqtt import robust
class MQTTClient(robust.MQTTClient):
def connect(self, clean_session=True):
res = super().connect(clean_session)
self.after_connect()
return res
def after_connect():
# use for mqtt subscribe
return
| Back | FazBrowse Home | New Git URL |
Not sure, why reconnect() call connect() of the base class (super().connect(False)).
Here are my reasons to change this: