Refactoring subscription API #108
Conversation
| @@ -213,7 +219,8 @@ mod test { | |||
| let packet = ReceivedPacket::from_buffer(&serialized_suback).unwrap(); | |||
| match packet { | |||
| ReceivedPacket::SubAck(sub_ack) => { | |||
| assert_eq!(sub_ack.code, ReasonCode::GrantedQos2); | |||
| assert_eq!(sub_ack.codes.len(), 1); | |||
There was a problem hiding this comment.
I think in general we should never panic on invalid messages. Just log/signal an error and continue.
If we can't skip through them without loosing packet sync, we should close the connection.
Panicing sounds decidedly wrong. But this is a general comment about all the code, not just this location here.
There was a problem hiding this comment.
This is specifically a unit test, so we want things to panic on failure. However, I agree with you. I do not believe we panic internally in the library anywhere (or it would be a bug if we did).
It's an interesting point that maybe the software should handle all errors by restting the client and just telling the user that a SessionReset occurred.
There was a problem hiding this comment.
Ah. I missed the context. All good!
This PR fixes #98 by allowing for a list of
TopicFilterobjects to be provided to thesubscribe()function. EachTopicFilteris composed ofSubscriptionOptionsand antopicpath. ConvenienceFrom<&str>has been implemented forTopicFilterto allow the user to subscribe similar to the previous API.@jordens There's some nuance in allowing the user to specify multiple TopicFilters per subscription request that ripples through the codebase. Let me know what you think about the change. We don't have to support multiple
TopicFilters perSUBSCRIBEif we don't want to.