search

adafruit / Adafruit_CircuitPython_GC_IOT_Core

Google Cloud IoT Core client for CircuitPython devices
Other
12 stars 12 forks source link

Change publish kwarg order to match other MiniMQTT API libraries #3

Open brentru opened 4 years ago

brentru commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@SAK917 brentru will be back next week

brentru commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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: https://github.com/adafruit/Adafruit_CircuitPython_AdafruitIO/pull/63

SAK917 commented 3 years ago

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!