Change publish kwarg order to match other MiniMQTT API libraries

[brentru]

The Publish method (https://github.com/adafruit/Adafruit_CircuitPython_GC_IOT_Core/blob/master/adafruit_gc_iot_core.py#L269) in this library should reflect the Publish(mqtt_topic, data, qos) structure of the Adafruit IO and AWS IoT CircuitPython libraries.

Method: https://github.com/adafruit/Adafruit_CircuitPython_AdafruitIO/blob/master/adafruit_io/adafruit_io.py#L364

[SAK917]

Hi @brentru, I thought I would try looking into some of the "Good First Issue" issues and saw this one. After researching it a bit I wanted to ask a question before I went too far. Seeing how it is a pretty old issue and the change will alter the existing API for the Publish method, I was wondering if you thought this change should still be made? I think changing the order of the parameters to match the structure of the Adafruit IO and AWS IoT CircuitPython libraries will potentially break any existing code that is using the current API:

Currently: def publish(self, payload, topic="events", subfolder=None, qos=0)
Adafruit IO: def publish(self, feed_key, data, metadata=None, shared_user=None, is_group=False)

If I understand the request correctly, we would want to swap the payload and topic="events" parameters like:

def publish(self, topic, payload, subfolder=None, qos=0)

but I believe that would require either making the topic parameter non-optional or making all the parameters optional, and either way would violate the existing API?

Please let me know if I am misunderstanding or making this harder than it really is. I am happy to do whatever needs to be done but need some confirmation that I am seeing things correctly... thanks!

[SAK917]

Hi @kattni, sorry to bug you but was hoping you could weigh in on my thoughts in the comment above? I am trying to knock out some of the older Issues but have concerns that the cat is already out of the bag on this one and changing the API at this point would not be desirable. I tried to reach out to brentru in my comment above, but so far no response. I would appreciate your thoughts on how best to proceed, or of this issue is moot at this point and might possibly be retired...

[ladyada]

@SAK917 brentru will be back next week

[brentru]

@SAK917 Yes, this change would break the existing API publish function.

But first, we'd need to do a breaking release for this library as it is (likely) incompatible with the latest version of MiniMQTT and CircuitPython 6 (https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT).

If you're interested in a "good first issue" - would you be interested in submitting a PR to update this library to work with the latest version of MiniMQTT?

[SAK917]

Good morning @brentru, didn't mean to seem pushy on getting a response, sorry. If you feel this is a good first issue (or fourth issue as is my case at this point) I am happy to do what I can to help! I am afraid I will need a bit more guidance however, since it isn't clear to me exactly what making this library work with the latest version of MiniMQTT entails...

[brentru]

it isn't clear to me exactly what making this library work with the latest version of MiniMQTT entails...

As an example, take a look at the pull request where I updated the Adafruit IO library: adafruit/Adafruit_CircuitPython_AdafruitIO#63

[SAK917]

The example will be a big help, I will digest it and get back to you with any questions. If things re clear, I will go ahead and start work on a new branch. Thanks for pointing me in the right direction and your patience!