-
Notifications
You must be signed in to change notification settings - Fork 179
Add default implementation and builder pattern for QoS #361
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
Conversation
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
jhdcs
left a comment
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.
These changes all look good to me! Only one minor comment/question about adding a comment to the default function, but I don't think it's enough to hold up the PR if we don't want to do it.
| } | ||
|
|
||
| impl Default for QoSProfile { | ||
| fn default() -> Self { |
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.
Should we make a note here that this is setting the QOS profile to the RCL default? Just to avoid any confusion?
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.
Done! 6ac6caa
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.
@luca-della-vedova thanks 🙂
esteve
left a comment
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.
@luca-della-vedova thanks. I agree with @jhdcs about adding a note about the RCL default, could you add that? Thanks
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
This PR adds a default implementation for
QoSProfileas well as a builder pattern based on therclcppAPI.I split it into a first commit that should be fully uncontroversial and a second commit that contains the builder API for
Durationbased items, which should also be OK I believe.I intentionally left out all the methods in the other APIs that just set a profile to the requested structure, since while in C++ people might have to do something like:
In Rust the API is (imho) not so necessary since they can just use struct initialization, such as: