Extend CLI daemon based features - #420
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
Running CI up to
|
| add_daemon_node_arguments(parser) | ||
| add_direct_node_arguments(parser) | ||
| parser.add_argument( | ||
| '--no-daemon', action='store_true', default=False, |
There was a problem hiding this comment.
Nitpick: default=False is not necessary. The default default value is None which would be fine.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
ivanpauno
left a comment
There was a problem hiding this comment.
LGTM!
A follow up replacing DirectNode by NodeStrategy where possible would be nice.
| server.register_function( | ||
| _print_invoked_function_name(node.get_service_names_and_types_by_node)) | ||
| server.register_function( | ||
| _print_invoked_function_name(node.get_client_names_and_types_by_node)) |
There was a problem hiding this comment.
count_publishers and count_subscribers (which are also graph related), should be added too.
There was a problem hiding this comment.
True, and I'll need it for ros2 topic info. Added in 5a1c313.
That's coming next. |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
One last sanity check:
|
|
I think these failures:
are occurring because ros2/sros2#173 was merged without this PR. Is there anything preventing this from being merged? |
|
Nope, it can be merged now. |
Connected to #419. This pull request:
--no-daemonoption forNodeStrategyclient code to use and allow users to NOT use nor spawn a daemon.