umqtt.robust: let reconnect() call the connect() method of the top class

[puuu]

Not sure, why reconnect() call connect() of the base class (super().connect(False)).

Here are my reasons to change this:

• umqtt.robust did not implement a new connect() method
• connect() can now be easily overwritten by a subclass. Allow, e.g., the implementation of a after_connect() method as suggested in #3186 (comment):

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

[ian-llewellyn]

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.

[ian-llewellyn]

@andrewleech, fancy reviewing / merging this PR? Do you see any pitfalls?

[andrewleech]

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.
Either your code with an empty after_connect function should be added to robust with a comment saying "subclass this add fill out the function", or provide a way for users to pass in a callback that will be run instead.

[ian-llewellyn]

I appreciate the review, and still think the PR, as is, has merit. Addressing the general thrust of your critique...

Strange and confusing

Agreed, 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 goal

Depending 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):

1. Minimum code size
2. Broker assumes all possible 'robustness' functions
3. An example to all of sub-classing umqtt.simple.MQTTClient
4. A convenient working client with some network fault tolerance out-of-the-box

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.

Outcomes

This 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.

Either your code with an empty after_connect function should be added to robust with a comment saying "subclass this add fill out the function", or provide a way for users to pass in a callback that will be run instead.

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.

[andrewleech]

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.