-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add SubscriptionInformation struct to usubscription #179
base: main
Are you sure you want to change the base?
Conversation
matthewd0123
commented
Aug 7, 2024
- In uStreamer, there will be a SubscriptionCache that is used to locally track subscriptions coming in from the subscription service. We needed to create a data structure to track these subscriptions, which I have defined here.
* In uStreamer, there will be a SubscriptionCache that is used to locally track subscriptions coming in from the subscription service. We needed to create a data structure to track these subscriptions, which I have defined here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at some suggestions. Overall would like to see more doc comments / doc tests
@@ -26,6 +26,41 @@ pub use crate::up_core_api::usubscription::{ | |||
|
|||
use crate::{UStatus, UUri}; | |||
|
|||
// Tracks information for the SubscriptionCache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use triple forward slashes for doc comments
// Tracks information for the SubscriptionCache | |
/// Tracks information for the SubscriptionCache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to expand this a bit with a code usage example a in triple tick block, e.g.
/// ```
/// // put the usage here
/// ```
that way it'll be compiled and tested.
} | ||
} | ||
|
||
impl Hash for SubscriptionInformation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding similar level of doc comment and doc example as below
} | ||
} | ||
|
||
impl Clone for SubscriptionInformation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to explicitly impl Clone
in this case, since all the members are just having .clone()
in turn called on them?
Reference this Rust Playground snippet.
Would it suffice to derive Clone
for SubscriptionInformation
as shown in the snippet?
|
||
impl Eq for SubscriptionInformation {} | ||
|
||
impl PartialEq for SubscriptionInformation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some comment explaining the choice to use subscriber
as the only member to check equality on would be good.
} | ||
} | ||
|
||
impl Hash for SubscriptionInformation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same sort of comment as above -- good to explain here why only the subscriber
is being used in the Hash.
Is this struct helpful to other users than uStreamer? If not, I do not see why it should go into up-rust... @PLeVasseur WDYT? |