-
Notifications
You must be signed in to change notification settings - Fork 55
Import message and support setting values of complex arrays/sequence #52
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: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
e314cba to
c1e25f2
Compare
| # TODO(sloretz) node API to get topic types should indicate if action or msg | ||
| middle_module = 'msg' | ||
| if topic_name.endswith('/_action/feedback'): | ||
| middle_module = 'action' |
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.
Once this problem has been addressed the API would not need to take topic_name anymore which would break users. Therefore I don't think this API should added as-is.
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 would expect in the future the import function to be something like https://github.com/ros2/rosidl_python/pull/33/files#diff-2283820db2117771f9254b0ff5ac87f0 using the abstract types.
What would be an acceptable API in the meantime? a keyword argument for the topic_name that would default to an empty string? (note that it would still break API the day that keyword argument is removed).
Or would you rather keep the original function in ros2topic and make a duplicated function here without this argument at all ?
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 would rather not add API here which is known that it needs to change as soon as something else gets fixed. So for now the API can stay were it is and once a stable API can be created it can land in the rosidl_runtime_py package.
Also I just created #54 which adds the SLOT_TYPES to each message. Maybe that is sufficient for what you want to achieve?
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.
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.
Yeah, that should work.
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.
|
Superseeded by #33 |
Version of #33 not changing the API exposed by message packages.
This is a redo of ros2/ros2cli#197 targetting this repository and updated to match what is returned by
get_fields_and_field_types.I left the tests commented out for now, they were passing about 2 days ago but since the message fixtures have been changed and the rosidl_runtime_py tests were not passing.Submitting for review without updated tests. Hopefully we can update the tests shortly once the test_msgs situation stabilizes
Tests have been updated, ready for review
tested using command from ros2/ros2cli#191 (comment):