-
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
Remove some code duplication between SequentialWriter and SequentialCompressionWriter #527
Conversation
from SequentialWriter. Created a new function prepare_to_open in SequentialWriter to reuse code with SequentialCompressionWriter Signed-off-by: Jaison Titus <jaisontj@amazon.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 LGTM - but it looks like it will need another pass to consolidate the logic in write
, so that the caching logic is not duplicated. In this case, it seems like the SequentialCompressionWriter isn't doing caching at all, that's how badly it's diverged.
rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp
Show resolved
Hide resolved
get_writeable_message which performs the operations on messages before writing it to storage or cache. Signed-off-by: Jaison Titus <jaisontj@amazon.com>
rosbag2_compression/src/rosbag2_compression/sequential_compression_writer.cpp
Outdated
Show resolved
Hide resolved
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.
Can you please add unit tests for new methods?
@dabonnie The new methods are non public and used internally as helper methods. Do we usually write unit tests for those? |
No, generally you unit test the public interface, not the implementation details (private methods). You can of course unit test protected methods by making a test-specific subclass of the given class, but that depends on how much you consider it an interface surface vs an implementation detail. |
overriding open Signed-off-by: Jaison Titus <jaisontj@amazon.com>
which open worked inside compressionwriter does not really matter Signed-off-by: Jaison Titus <jaisontj@amazon.com>
a28670d
to
23f1281
Compare
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.
Thanks for iterating, this is much cleaner!
…ompressionWriter (#527) Remove code duplication between SequentialWriter and SequentialCompressionWriter Signed-off-by: Jaison Titus <jaisontj@amazon.com>
…ompressionWriter (#527) Remove code duplication between SequentialWriter and SequentialCompressionWriter Signed-off-by: Jaison Titus <jaisontj@amazon.com>
…ompressionWriter (ros2#527) Remove code duplication between SequentialWriter and SequentialCompressionWriter Signed-off-by: Jaison Titus <jaisontj@amazon.com>
[Edited] Major changes:
SequentialCompressionWriter
now inherits fromSequentialWriter
get_writeable_message
inside ofSequentialWriter
to be called to get the message to be written to storage or cache