-
Notifications
You must be signed in to change notification settings - Fork 158
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
Added ros2interface to replace ros2 msg/srv #288
Conversation
…into kucheria/interface Current branch is stale, need to rebase with master
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 can be some refactoring done. Most of the functions should work for generic IDL files, and don't need to make the distinction between "msg", "srv", and "action". For options like --msg-only
, we can filter the list based on the second namespace.
This might be a good time to move some of the rosidl-specific logic to another package. Perhaps as a follow-up once we have the functionality working here.
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.
Looking good. In addition to addressing the comments below, I think it would be nice to add some unit tests for the functions in ros2interface/api
.
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.
To improve test coverage, it would be nice to add more API tests for the remaining functions.
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, just some nitpicks and small fixes.
Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
Co-Authored-By: Jacob Perron <jacob@openrobotics.org>
|
||
def get_interface_path(parts): | ||
prefix_path = has_resource('packages', parts[0]) | ||
joined = '/'.join(parts) |
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 should be a platform specific separator.
return list(sorted({ | ||
n[4:-4] | ||
for n in interface_names | ||
if n.startswith('srv/') and n[-4:] in ('.idl', '.srv')})) |
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.
Several of these function look like copy-n-paste with only minimal differences. Please refactor to avoid the duplication. Also we do have a rosidl_runtime_py
now so all the TODOs can be addressed.
Signed-off-by: Aditya <aditya050995@gmail.com> Co-authored-by: Aditya <aditya050995@gmail.com>
Addressing #223