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.

Sign up for GitHub

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

Jump to bottom

Expand timestamps for std_msgs.msg.Header and builtin_interfaces.msg.Time if 'auto' and 'now' are passed as values #19

Merged
merged 4 commits into from
Sep 13, 2022
Merged

Expand timestamps for std_msgs.msg.Header and builtin_interfaces.msg.Time if 'auto' and 'now' are passed as values #19

merged 4 commits into from
Sep 13, 2022

Conversation

esteve
Copy link
Member

@esteve esteve commented Sep 2, 2022 •
edited
Loading

Merge the logic from ros2/ros2cli#749 into set_message_fields

Closes ros2/ros2cli#700

@esteve esteve mentioned this pull request Sep 2, 2022
Merged
@esteve esteve changed the title Expand timestamps for std_msgs.msg.Header and builtin_interfaces.ms.Time if 'auto' and 'now' are passed as values Expand timestamps for std_msgs.msg.Header and builtin_interfaces.msg.Time if 'auto' and 'now' are passed as values Sep 2, 2022
ivanpauno
ivanpauno approved these changes Sep 2, 2022
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

The change seems fine to me, it's backwards compatible, even if it adds a return value to the function.
It would be good to move the test cases here as well.

I will tag one of the maintainers of this package for review.

@ivanpauno ivanpauno added the enhancement New feature or request label Sep 2, 2022
@esteve
Expand timestamps for std_msgs.msg.Header and builtin_interfaces.msg.…
22bb48c 
…Time if 'auto' and 'now' are passed as values

Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@esteve esteve force-pushed the set-message-fields-expand-auto-now branch from 8366636 to e6c8d6d Compare September 5, 2022 10:25
@esteve
Copy link
Member Author

esteve commented Sep 5, 2022

@ivanpauno I've updated this PR with the tests from ros2/ros2cli#749

@esteve esteve force-pushed the set-message-fields-expand-auto-now branch from e6c8d6d to 5250ba5 Compare September 5, 2022 10:26
@esteve esteve mentioned this pull request Sep 5, 2022
Merged
ivanpauno
ivanpauno approved these changes Sep 5, 2022
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM

@esteve
Added tests from ros2/ros2cli#749
167cd76 
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@esteve esteve force-pushed the set-message-fields-expand-auto-now branch from 5250ba5 to 167cd76 Compare September 7, 2022 09:42
@ivanpauno
Copy link
Member

@sloretz could you take a look to this PR?

sloretz
sloretz reviewed Sep 9, 2022
View reviewed changes
Copy link

@sloretz sloretz left a comment

Choose a reason for hiding this comment

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

This looks really useful! I recommend adding test cases for when now and auto are used but expand_header_auto and expeand_time_now are False, but I won't block on that.

@esteve esteve marked this pull request as draft September 12, 2022 09:06
@esteve
Copy link
Member Author

esteve commented Sep 12, 2022

@sloretz sounds good, I'll add those tests here and in #20

@esteve
Added tests for unparsed now and auto keywords
5c10bf6 
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@esteve esteve marked this pull request as ready for review September 12, 2022 14:33
@esteve
Copy link
Member Author

esteve commented Sep 12, 2022

@sloretz I've added the tests in 5c10bf6

@esteve esteve mentioned this pull request Sep 12, 2022
Merged
@ivanpauno
Copy link
Member

ivanpauno commented Sep 12, 2022 •
edited
Loading

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ivanpauno
Copy link
Member

@esteve could you fix the flake8 failures?

@esteve
Fix flake8 error
9e07e52 
Signed-off-by: Esteve Fernandez <esteve.fernandez@tier4.jp>
@esteve
Copy link
Member Author

esteve commented Sep 13, 2022

@ivanpauno done in 9e07e52 The import order does not look pretty IMHO, but it makes flake8 happy.

@ivanpauno
Copy link
Member

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ivanpauno ivanpauno merged commit 0d7c227 into ros2:rolling Sep 13, 2022
@esteve esteve mentioned this pull request Oct 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloretz sloretz sloretz left review comments

@ivanpauno ivanpauno ivanpauno approved these changes

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ros2 topic pub auto header
4 participants
@esteve @ivanpauno @sloretz @fujitatomoya