Add compression factory implementation #313
Conversation
rosbag2_compression/src/rosbag2_compression/compression_factory_impl.hpp
Outdated
Show resolved
Hide resolved
rosbag2_compression/src/rosbag2_compression/compression_factory_impl.hpp
Outdated
Show resolved
Hide resolved
rosbag2_compression/test/rosbag2_compression/test_compression_factory.cpp
Show resolved
Hide resolved
piraka9011
requested review from
zmichaels11,
prajakta-gokhale,
dabonnie and
emersonknapp
rosbag2_compression/src/rosbag2_compression/compression_factory_impl.hpp
Outdated
Show resolved
Hide resolved
|
Lets wait on #314 (review) to be merged before merging in this (with CI) |
|
Merged #314 in so this PR is no longer blocked |
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
Signed-off-by: Anas Abou Allaban <aabouallaban@pm.me>
piraka9011
force-pushed
the
split-comp-fix
branch
from
91f33e6
to
a78c73d
Compare
|
@ros2/aws-oncall @prajakta-gokhale @thomas-moulard - please run this CI job |
|
rosbag2_compression/test/rosbag2_compression/test_compression_factory.cpp
Outdated
Show resolved
Hide resolved
rosbag2_compression/src/rosbag2_compression/compression_factory_impl.hpp
Show resolved
Hide resolved
| namespace | ||
| { | ||
|
|
||
| constexpr const char kCompressionFormatZstd[] = "zstd"; |
There was a problem hiding this comment.
Why anonymous? If not, then others can use and in the tests as well. Or another question, why not use the identifier provided by the zstd compressor?
There was a problem hiding this comment.
I'd need to make get_compression_identifier() static which I'd prefer to do in another PR. But yes that would make this less repetitive.
There was a problem hiding this comment.
I don't think you can make BaseCompressorInterface::get_compression_identifier static since it needs to be implemented by each supported compression. You can add a static function to ZstdCompressor
| TEST_F(CompressionFactoryTest, creates_zstd_compressor_caseinsensitive) | ||
| { | ||
| const auto compression_format = "ZsTd"; | ||
| const auto lowercase_compression_format = to_lower(compression_format); |
There was a problem hiding this comment.
Why reimplement case_insensitive_compare?
There was a problem hiding this comment.
Not sure I follow this one. I need to verify that the compressor created is the correct one.
Are you suggesting that I instead use case_insensitive_compare to compare the compression format?
There was a problem hiding this comment.
This might be resolved by making get_compression_identifier() static as well.
| TEST_F(CompressionFactoryTest, creates_zstd_compressor_uppercase) | ||
| { | ||
| const auto compression_format = "ZSTD"; | ||
| const auto lowercase_compression_format = to_lower(compression_format); |
| TEST_F(CompressionFactoryTest, creates_zstd_decompressor_caseinsensitive) | ||
| { | ||
| const auto compression_format = "ZsTd"; | ||
| const auto lowercase_compression_format = to_lower(compression_format); |
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
Signed-off-by: Anas Abou Allaban <allabana@amazon.com>
|
@ros2/aws-oncall - please run this CI job |
|
Add implementation of the
CompressionFactory.Signed-off-by: Anas Abou Allaban aabouallaban@pm.me