refactor will_set function to match the publish function and allow the msg/payload to be encoded bytes, not just str, int or float. #221
Conversation
.pre-commit-config.yaml
Outdated
| hooks: | ||
| - id: black | ||
| - repo: https://github.com/fsfe/reuse-tool | ||
| rev: v1.1.2 |
There was a problem hiding this comment.
I don't think we want to change all of these versions in the pre-commit config file.
Ideally we try to keep these in sync across all of the libraries to make maintenance easier. Then we can use a patch to update them all at once.
However, I do know there are issues using the pinned version of pylint in python 3.12 and updating it to 3.x does allow it to work in that environment.
We're also in the process of switching the libraries over to use Ruff instead of pylint and Black.
I think it's best to revert these changes to the pre-commit file for now and the ruff change-over for this can be submitted as a separate PR since it's unrelated to main functional change in this PR.
There was a problem hiding this comment.
Hi @FoamyGuy , thank your, and my apologies for being thick but I don't think I intentionally changed this file. Are you asking me to make a new pull request or am I just not understanding you? Sorry for my lack of clarity :-)
There was a problem hiding this comment.
@ehagerty no worries. It seems that those were changed in your branch accidentally. A new PR is not needed, just changing them back to the original values that they were for now.
I've added a commit to this branch just now to change them back so it is taken care of.
I've been reading up in the MQTT spec and I am in agreement with your conclusion that the last message can be a normal message just like any other. I'm going to give this a quick test today to ensure no other issues with it.
There was a problem hiding this comment.
This looks good to me.
I tested on a Feather S3 TFT with AdafruitIO using the examples in the repo. Everything appears to work as expected.
I think it makes sense to match the argument types for publish with the ones on will_set since they do seem to support the exact same kinds of messages.
I checked for existing usages of the will_set() in the learn guide repo and found only 1 usage which doesn't pass arguments by name so it should be un-affected by this change.
Updating https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT to 7.10.1 from 7.10.0: > Merge pull request adafruit/Adafruit_CircuitPython_MiniMQTT#221 from ehagerty/will_set_bytes Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
As far as I can understand in reading the specification and talking with our mqtt broker host, there is no reason to disallow the last_will message being sent as bytes (encoded json) in the same way that you are supposed to for all other publish operations. As the last_will is still 'just' a publish operation, albeit a delayed one, this seems logical. Perhaps more controversially, I have changed the use of 'payload' as a key name in last_will to also match the publish function and use 'msg' for the same content. I did so because I believe it will help prevent any future confusion.