-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Refactor using a combined QosPid
struct.
#11
Comments
vincentdephily
added a commit
to vincentdephily/mqttrs
that referenced
this issue
Oct 16, 2019
This avoids the gotcha (both for mqttrs and its users) of setting AtMostOnce with a pid, or AtLeastonce/ExactlyOnce without one. Fixes 00imvj00#11 Breaking change.
vincentdephily
added a commit
to vincentdephily/mqttrs
that referenced
this issue
Oct 16, 2019
This avoids the gotcha (both for mqttrs and its users) of setting AtMostOnce with a pid, or AtLeastonce/ExactlyOnce without one. Fixes 00imvj00#11 Breaking change.
vincentdephily
added a commit
to vincentdephily/mqttrs
that referenced
this issue
Oct 17, 2019
This avoids the gotcha (both for mqttrs and its users) of setting AtMostOnce with a pid, or AtLeastonce/ExactlyOnce without one. Fixes 00imvj00#11 Breaking change.
Probably want to wait until the branch is merged before closing ;) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Related to bug #6, but deserves to be treated separately as it is an API change instead of a bugfix:
In my own code I've found it useful to have both a
and a
The QosPid struct would have made bug #6 more obvious, and would prevent similar bugs in user code (where it's easy to not have
qos
andpid
in sync).I suggest to do the same in this lib, replace the two fields
qos:QoS, pid:Option<PacketIdentifier>
with a singleqospid: QosPid
as defined above. This goes towards the "hard to misuse" goal of good APi design (and makes the struct smaller as a bonus).Like #10, this is a breaking change. Hopefully we can lump a few together.
The text was updated successfully, but these errors were encountered: