Remove explicit sqlite3 from code #1166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emersonknapp
requested review from
gbiggs and
hidmic
and removed request for
a team
james-rms
reviewed
Nov 18, 2022
MichaelOrlov
approved these changes
Nov 20, 2022
There was a problem hiding this comment.
LGTM. Among one question in rosbag2_py/test/test_convert.py
Removes all code mentions of "sqlite3" from packages * rosbag2_cpp * rosbag2_performance_benchmarking * rosbag2_py * rosbag2_storage * rosbag2_transport `rosbag2_tests` and the CLI/README will be in separate PRs Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
emersonknapp
force-pushed
the
emersonknapp/remove-sqlite3-explicit-code
branch
from
f49f45a
to
9afe8bc
Compare
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
james-rms
reviewed
Nov 21, 2022
| @@ -579,7 +580,7 @@ TEST_F(TemporaryDirectoryFixture, split_bag_metadata_has_full_duration) { | |||
| }; | |||
| rosbag2_storage::StorageOptions storage_options; | |||
| storage_options.uri = (rcpputils::fs::path(temporary_dir_path_) / "split_duration_bag").string(); | |||
| storage_options.storage_id = "sqlite3"; | |||
| storage_options.storage_id = rosbag2_storage::get_default_storage_id(); | |||
There was a problem hiding this comment.
Would it make sense to leave this as hardcoded, with the aim to parametrize this test across both ("sqlite3", "mcap") when both are installed in the CI environment?
There was a problem hiding this comment.
I think that leaving it hardcoded doesn't get us any closer to that parametrization - so this is at worst a sideways move and at best removes an explicit mention, so I'd rather leave this PR this way.
james-rms
reviewed
Nov 21, 2022
| @@ -46,7 +47,7 @@ TEST_F(MetadataFixture, test_writing_and_reading_yaml) | |||
| { | |||
| BagMetadata metadata{}; | |||
| metadata.version = 1; | |||
| metadata.storage_identifier = "sqlite3"; | |||
| metadata.storage_identifier = get_default_storage_id(); | |||
There was a problem hiding this comment.
Same comment here as with test_sequential_writer
|
Gist: https://gist.githubusercontent.com/emersonknapp/5527da6c5a57cbc6ac31537c4702c2a0/raw/64cef56d4cda9be4154fc9a427641a1a52a08b54/ros2.repos
|
delete-merged-branch
bot
deleted the
emersonknapp/remove-sqlite3-explicit-code
branch
|
https://github.com/Mergifyio backport humble |
✅ Backports have been created
|
mergify bot
pushed a commit
that referenced
this pull request
Jun 22, 2024
* Remove explicit sqlite3 from code Removes all code mentions of "sqlite3" from packages * rosbag2_cpp * rosbag2_performance_benchmarking * rosbag2_py * rosbag2_storage * rosbag2_transport `rosbag2_tests` and the CLI/README will be in separate PRs Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> (cherry picked from commit 3300a27) # Conflicts: # rosbag2_cpp/test/rosbag2_cpp/test_info.cpp # rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp # rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp
MichaelOrlov
pushed a commit
that referenced
this pull request
Jun 23, 2024
* Remove explicit sqlite3 from code Removes all code mentions of "sqlite3" from packages * rosbag2_cpp * rosbag2_performance_benchmarking * rosbag2_py * rosbag2_storage * rosbag2_transport `rosbag2_tests` and the CLI/README will be in separate PRs Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> (cherry picked from commit 3300a27) # Conflicts: # rosbag2_cpp/test/rosbag2_cpp/test_info.cpp # rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp # rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp
MichaelOrlov
added a commit
that referenced
this pull request
Jun 23, 2024
* Remove explicit sqlite3 from code (#1166) * Remove explicit sqlite3 from code Removes all code mentions of "sqlite3" from packages * rosbag2_cpp * rosbag2_performance_benchmarking * rosbag2_py * rosbag2_storage * rosbag2_transport `rosbag2_tests` and the CLI/README will be in separate PRs Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com> (cherry picked from commit 3300a27) # Conflicts: # rosbag2_cpp/test/rosbag2_cpp/test_info.cpp # rosbag2_cpp/test/rosbag2_cpp/test_sequential_reader.cpp # rosbag2_cpp/test/rosbag2_cpp/test_sequential_writer.cpp * Address merge conflicts - Shall be applied above backport of the #1146. Due to missing `default_storage_id.hpp` Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai> Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com> Co-authored-by: Michael Orlov <michael.orlov@apex.ai>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes all code mentions of "sqlite3" from packages
rosbag2_testsand the CLI/README will be in separate PRs