-
Notifications
You must be signed in to change notification settings - Fork 129
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
Expose .msg/.srv/.action to .idl conversion via rosidl translate CLI #576
Conversation
26e2bd0
to
38748ff
Compare
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
91e5152
to
014e218
Compare
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.
LGTM with a couple nitpicks and windows CI made happy
rosidl_adapter/rosidl_adapter/cli.py
Outdated
@@ -1,4 +1,4 @@ | |||
# Copyright 2018 Open Source Robotics Foundation, Inc. | |||
# Copyright 2018-2021 Open Source Robotics Foundation, Inc. |
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.
No need to update the copyright year. I'm pretty sure we don't usually bump it.
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've seen both, but I don't mind strongly. Reverted in 74e0044.
rosidl_adapter/rosidl_adapter/cli.py
Outdated
|
||
@property | ||
def conversion_function(self): | ||
from rosidl_adapter.msg import convert_msg_to_idl |
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.
Why is the import in the function instead of on the module?
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.
This source file didn't import them directly, so I just went with it. But I don't mind strongly. Moved to the top in 74e0044.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Re-running Windows CI after 2775f03: |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Re-running Windows CI after 928c1a4: |
And again, using ament/ament_cmake#327: |
CI's green and ament/ament_cmake#327 is merged. Going in ! |
- Fix for ""// with input from rosbag2_storage_mcap_testdata/msg\\ComplexMsgDependsOnIdl.msg" - On Windows platform message generator messing up with the input file name ""/msg\\ComplexMsgDependsOnIdl.msg" by inserting double backslashes instead of one forward slash. - Partial backport from ros2#576 Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
Connected to #565. Depends on #575.