Skip to content
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

Make PacketType the size of u8 #54

Merged
merged 2 commits into from
Mar 29, 2021
Merged

Conversation

coolreader18
Copy link
Contributor

I noticed that methods on ConnectPacket weren't properly setting flags values (set_dup(false) wouldn't clear the bit), and I also realized it should be possible to pack it into one byte instead of 2.

/// ControlType as defined by the [MQTT spec].
///
/// [MQTT spec]: http://docs.oasis-open.org/mqtt/mqtt/v3.1.1/os/mqtt-v3.1.1-os.html#_Table_2.2_-
pub fn new(t: ControlType, flags: u8) -> Option<PacketType> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't be Option. You may want to use Result instead.

@zonyitoo
Copy link
Owner

zonyitoo commented Mar 28, 2021

@coolreader18 What do you think? new should return Result, right?

It has been a while since last release. I should publish a new version.

@coolreader18
Copy link
Contributor Author

Well I originally had it returning Result<, PacketTypeError>, but I realized that it only had one error condition, and given that you passed the flags you probably didn't need access to them in the error as well. However I could just have a unit error type, or one that just wraps the flags.

@zonyitoo
Copy link
Owner

zonyitoo commented Mar 28, 2021

Although it only is an unit error type, but it should be an Err in this case.

A wrapper is better for implementing Debug for this error type, I think.

@zonyitoo zonyitoo merged commit 8c8766d into zonyitoo:master Mar 29, 2021
@coolreader18 coolreader18 deleted the pktyp-byte branch April 21, 2021 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants