comply with new node representation #149
Conversation
|
Yes, I was supposed to follow up on that, I didn't realize that #146 got merged. Sorry for the inconvenience. |
| if args.node_name in node_name.full_name: | ||
| node_found = True | ||
| break | ||
| if not node_found: |
There was a problem hiding this comment.
This could be much shorter: if args.node_name not in {n.full_name for n in node_names}:
There was a problem hiding this comment.
of course there is a one-liner for python :) thanks!
| @@ -27,14 +27,14 @@ def get_node_names(*, node, include_hidden_nodes=False): | |||
| node=node, include_hidden_nodes=include_hidden_nodes) | |||
| service_names_and_types = get_service_names_and_types(node=node) | |||
| return [ | |||
| n for n in node_names | |||
| if _has_lifecycle(n, service_names_and_types)] | |||
| n.full_name for n in node_names | |||
There was a problem hiding this comment.
Would it be more consistent with the same function make in ros2node to return the "full" n here and let the callers pick thefull_name from it?
There was a problem hiding this comment.
I can change it if you want. But given that this function anyway only really serves for the lifecycle implementation i didn't do it at first.
Also I feel that get_node_names is then a bit misleading, because it really is a struct with more than just the name of the node.
There was a problem hiding this comment.
I found it already confusing that within this patch the usage is different, e.g. ros2lifecycle/verb/set.py uses the API from ros2node.
I think the
ros2lifecycleis currently not working currently since #146 got merged.