-
Notifications
You must be signed in to change notification settings - Fork 251
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
Add --start-paused option to ros2 bag play
#904
Conversation
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
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 with green CI, no concerns. Thanks for the upgrade!
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
This cppcheck error looks like a false positive:
I've suppressed it in 32caf26, if another approach is prefered (e.g. renaming the argument), let me know and I will address that. |
Another option would be to name the ctor parameter |
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
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 looks good to me code-wise. Only two (minor) comments on help text and variable names, but I'm not sure if we want to hold the PR back because of it.
Basically, I'm just trying to think about potential confusion points for future users and maintainers.
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
CI is passing, @jhdcs @emersonknapp let me know if this can be merged or if something else is missing. |
I think this was proposed in #443.
That PR ended up being closed in favor of #696, but an option to start the player in a paused state hasn't been added AFAIS.