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.
There was a problem hiding this comment.
Done, I think eventually we may want to rename Error to ReadError, and also re-organize the crate so that Reader and Writer both live in similar sub-module / folder layouts. But trying to keep changes in this MR minimal.
| 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.
|
@newpavlov Sorry for delay here, was expecting to use this for a project for work, but that got delayed. I've updated with your suggested changes (all good ideas!) and I think this is ready for a final review / merge now. Thanks again for joining this crate with RosLibRust org. if you've got time to continue to review I'd appreciate it. I also don't have crates.io ownership / publish rights yet. If you want to send those my way I'd be happy to accept. Otherwise happy to continue to have you publish. |
You listed as an owner of the crate. BTW it may be worth to configure Trusted Publishing for this crate. |
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.