-
Notifications
You must be signed in to change notification settings - Fork 254
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
initial command line interface #12
Conversation
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.
Is this intended for review and merge?
I don't feel like what's here needs review. It doesn't do anything but put the arguments it's given, so it's more like a skeleton for future work. I guess the verb's you've selected are ok (info, record, play), so I'd say just merge it without review until there's a larger (or trickier) chunk of code to review.
ros2bag/ros2bag/api/__init__.py
Outdated
@@ -0,0 +1,17 @@ | |||
# Copyright 2018 Open Source Robotics Foundation, Inc. |
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.
Could this file just be empty and the dependency on rclpy removed from the package.xml ?
Requesting feedback on the verbs and arguments is good. Until this patch contain any actual functionality I would suggest not to merge it yet. Simply because users building from source might be confused about the presence of the command but not doing anything. |
Well, they already added empty packages (see this as similar skeleton work). I don't think this repository is part of the default package list, so that mitigates it as well. I was trying to accommodate @Karsten1987's expressed desired to merge more frequent and smaller pr's (I think he mentioned that at some point). @Karsten1987 you could add a disclaimer to all the commands for now, or merge into a working branch until you're ready for something on 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.
lgtm,
the pytest dependency is unused but I assume that this package will have tests soon so I guess it's fine to keep it.
Remove uncrustify linter in favor of clang-format, which is easier to configure for use in VS Code format-on-save. Signed-off-by: Jacob Bandes-Storch <jacob@foxglove.dev>
Remove uncrustify linter in favor of clang-format, which is easier to configure for use in VS Code format-on-save. Signed-off-by: Jacob Bandes-Storch <jacob@foxglove.dev> Signed-off-by: James Smith <james@foxglove.dev>
Provides entry points for record, play, info