Full Implementation of RosBagWriter#8
Conversation
Carter12s
force-pushed
the
implement-rosbag-writer
branch
from
65e8821 to
cdece3f
Compare
|
I ended up doing a bunch of CI runs over here: Carter12s#1 And CI is currently passing on my Repo, but I did have to do some fairly gross gymnastics to work around the MSRV of this project. I'm inclined to increase the MSRV in the release that add the Writer, but open to thoughts. This PR also adds a devcontainer setup to simplify local environment setup as well as a CI setup with a ROS noetic installation to enable some integration testing. Happy to remove those or move to separate PRs. |
newpavlov
left a comment
newpavlov
left a comment
There was a problem hiding this comment.
I haven't reviewed the writer implementation itself. Just several minor suggestions.
|
|
||
| // Write-specific errors | ||
| /// I/O error during write operations. | ||
| IoError(std::io::Error), |
There was a problem hiding this comment.
IMO it's better to introduce a separate error type for the writer.
| pub use write_cursor::WriteCursor; | ||
|
|
||
| #[cfg(test)] | ||
| mod integration_tests { |
There was a problem hiding this comment.
Personally, I prefer to keep integration tests in the tests/ folder and use unit tests mostly for testing private APIs.
Same for other tests in the sub-modules.
| profile: minimal | ||
| # Pin dependencies for MSRV compatibility (Rust 1.63) | ||
| # Order matters: pin high-level crates before their transitive dependencies | ||
| - name: Pin MSRV-compatible dependency versions |
There was a problem hiding this comment.
I think it's fine to just bump MSRV, update edition to 2024, and rely on the MSRV-aware resolver going forward.
2 participants
Addresses #3
Took a full swing at it, and did pretty thorough testing so I think the implementation is sound. Not very familiar with bag file format before this and the documentation is a little barren, so I hope I didn't miss anything major.