New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change publish kwarg order to match other MiniMQTT API libraries #3
Comments
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 Currently: If I understand the request correctly, we would want to swap the
but I believe that would require either making the 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! |
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... |
@SAK917 brentru will be back next week |
@SAK917 Yes, this change would break the existing API 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? |
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... |
As an example, take a look at the pull request where I updated the Adafruit IO library: adafruit/Adafruit_CircuitPython_AdafruitIO#63 |
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! |
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
The text was updated successfully, but these errors were encountered: