-
Notifications
You must be signed in to change notification settings - Fork 240
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
introduce defaults #452
introduce defaults #452
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.
These seem like reasonable defaults to me
@emersonknapp sorry, I've added one other thing to this PR. I introduce an overload for |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-07-16/15468/1 |
{ | ||
rosbag2_cpp::StorageOptions storage_options; | ||
storage_options.uri = uri; | ||
storage_options.storage_id = "sqlite3"; |
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.
minor nit - should these 3 default values ("sqlite3", 0, 0) be defined as constants somewhere?
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.
the last two are directly "hardcoded" in the storage options struct - meaning I could technically leave out the following two lines of code.
Where would you define the other constant, "sqlite3"?
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.
@emersonknapp I've excluded the sqlite3
into a constant within the same file. Let me know if that works for you.
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
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
Signed-off-by: Karsten Knese <karsten@openrobotics.org>
* minimal c++ API test Signed-off-by: Karsten Knese <karsten@openrobotics.org> * introduce defaults Signed-off-by: Karsten Knese <karsten@openrobotics.org> * open overload for simple uri string Signed-off-by: Karsten Knese <karsten@openrobotics.org> * set sqlite3 as a constant Signed-off-by: Karsten Knese <karsten@openrobotics.org>
* minimal c++ API test Signed-off-by: Karsten Knese <karsten@openrobotics.org> * introduce defaults Signed-off-by: Karsten Knese <karsten@openrobotics.org> * open overload for simple uri string Signed-off-by: Karsten Knese <karsten@openrobotics.org> * set sqlite3 as a constant Signed-off-by: Karsten Knese <karsten@openrobotics.org>
* minimal c++ API test Signed-off-by: Karsten Knese <karsten@openrobotics.org> * introduce defaults Signed-off-by: Karsten Knese <karsten@openrobotics.org> * open overload for simple uri string Signed-off-by: Karsten Knese <karsten@openrobotics.org> * set sqlite3 as a constant Signed-off-by: Karsten Knese <karsten@openrobotics.org>
This PR sits on top of #451 to show case the difference when decent defaults are chosen.
I'd like to hear from others whether these defaults are sensible or not.