Changed is_connected to return bool while maintaining error checking

[BiffoBear]

closes #109
Renamed original is_connected method to _connected to maintain internal connection status checking. Created a new is_connected function that returns a bool. This is a breaking change.

This change allows code to be written like:

while not mqtt_client.is_connected():
    try:
        mqtt_client.connect()
    except RuntimeError:
        print("Unable to connect. Waiting for 10 seconds before retrying…")
        time.sleep(10)
print("Connected to MQTT broker.")

[tekktrik]

I think this is a good change, but I'm also not as familiar with MQTT applications. I think this is worth getting a second opinion since its also API breaking. I'll bring it up at the CircuitPython weekly meeting tomorrow!

[BiffoBear]

I think this is a good change, but I'm also not as familiar with MQTT applications. I think this is worth getting a second opinion since its also API breaking. I'll bring it up at the CircuitPython weekly meeting tomorrow!

After poking around in the code, I believe the is_connected() method is poorly named. The method does not reflect the state of the connection when it is called. It checks whether a network socket exists and whether an initial connection was successfully made to the MQTT broker when MQTT.connect() was called. The is_connected method acts as a gatekeeper so methods like publish() and ping() are not called before the connection to the broker has been made. This is why it returns True or raises an exception.

If the MQTT broker is down, the is_connected method will return True but any attempt to write to the broker will raise a BrokenPipeError: 32 exception.

After poking the code a bit more, MQTT.connect() returns a value if a connection with the broker was successfully initiated and raises exceptions for failed connections. Thus a while not mqtt_client.is_connected: loop is not required.
In short, I no longer think that this breaking change is worth making.

Perhaps the method should be renamed _initial_connection_made() or something along those lines to make clear that it does not reflect the current connection status and is not particularly useful to the user.

[jepler]

Thanks! However, I think it could be written more simply. Do you mind changing it around?

[BiffoBear]

Thanks to @2bndy5 for clarifying my thinking and pointing out that the Paho implementation returns a bool. Changes requested made.