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

Adding a simple helper function for converting rclrs::Time to a ros message #359

Merged
merged 4 commits into from
Jan 12, 2024

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Jan 10, 2024

This commit adds a simple conversion function that converts Time to builtin_interfaces::msg::Time like the one in rclpy.

message

This commit adds a simple conversion function that would converts Time to
builtin_interfaces like the one in rclpy.

Signed-off-by: Arjo Chakravarty <[email protected]>
@jhdcs jhdcs self-requested a review January 10, 2024 13:36
Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

I like this! Nice and simple, easy to understand, and helps the end user to do their job more efficiently. Thank you!

Only catch is that there appears to be some formatting issues that the linter isn't happy about. Would you be able to modify your submission so that the linter lets the rest of the tests run? Apart from that, this looks pretty good to me!

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 requested a review from jhdcs January 11, 2024 02:15
esteve
esteve previously approved these changes Jan 11, 2024
Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

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

@arjo129 thank you so much! This looks great, I only have one minor style request, but I've approved this PR anyway so that it can be merged

rclrs/Cargo.toml Outdated Show resolved Hide resolved
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129
Copy link
Contributor Author

arjo129 commented Jan 12, 2024

Looks like my last commit dismissed the review 🤷 . I'd be happy to make similar PRs for Duration and the time going the other way.

Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

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

@arjo129 thanks! @jhdcs do you have more feedback?

Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

Nope! Looks good to me!

Thank you very much for the contribution!

@esteve esteve merged commit 164631e into ros2-rust:main Jan 12, 2024
3 checks passed
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.

3 participants