parse middle_module from message type in ros2topic #278
Conversation
support either .msg or .idl packages Signed-off-by: David Hodo <[email protected]>
|
It's undecided if we'll allow message types with more than three parts. There's an open proposal that there could be more than three: ros2/design#220 |
Signed-off-by: David Hodo <[email protected]>
| package_name, *message_name = message_type.split('/') | ||
| if not package_name or not message_name or not all(message_name): | ||
| package_name, middle_module, message_name = message_type.split('/') | ||
| if not middle_module: |
There was a problem hiding this comment.
This logic doesn't seem to make any sense. In which case would not middle_module be True?
| raise ValueError() | ||
| except ValueError: | ||
| raise RuntimeError('The passed message type is invalid') | ||
|
|
||
| # 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' | ||
|
|
||
| module = importlib.import_module(package_name + '.' + middle_module) |
There was a problem hiding this comment.
Perhaps we should just support any number of parts, e.g. '.'.join([package_name] + message_name[:-1]).
There was a problem hiding this comment.
Does it not generally make sense to unify this logic and have it in a central place? I know of at least one more other location where we duplicate the same logic:
https://github.com/ros2/ros2cli/blob/master/ros2service/ros2service/verb/call.py#L61-L72
There was a problem hiding this comment.
Yes. There's a TODO at the top of this function. I'm not sure what package it should belong in (or if a new package needs to be created).
|
@davidhodo will you be able to address the feedback on this PR? |
|
Yes. Sorry for the long delay. I will try to get it done this week. |
|
@davidhodo are you still interested in updating this PR? |
|
I think this PR is obsolete by #415. I'll check if #277 is resolved by #415 and if not, we should probably target a PR against https://github.com/ros2/rosidl_runtime_py. |
|
If we want this fix for Dashing, then I suggest re-opening this PR and changing the base branch to |
|
Sorry. I am still interested, but have been swamped and haven't been able to get back to it. I'll rebase on Dashing and re-open as soon as I get some time. |
Signed-off-by: Shane Loretz <[email protected]>
Attempts to solve issue #277. The middle module name is parsed from the message type rather than being hardcoded as .msg. I have tested it with a few message types generated from .msg and from .idl but not exhaustively. Is there a case where the message_type split could result in more than 3 strings?