Update library and add tests #9
Conversation
|
Hello @mattcieslak -- thanks for all your work here! A word of warning (that maybe should have appeared more prominently in the README...) is that the software is currently in rather early stages of development (and as you can see from the commit log a bit stalled). That said, if you could make it work through your efforts, that would be absolutely fantastic, and would really help push adoption of TRX. For the time being, I will activate copilot review and take a look. My C++ is very very rusty and knowledge full of holes, so my review would be rather limited, but let's also see whether copilot gives us anything useful. |
There was a problem hiding this comment.
Pull request overview
This pull request updates the trx-cpp library to make it compilable and adds comprehensive test coverage. The changes include refactoring the zip file loading mechanism to extract files to a temporary directory, improving error messages, fixing compilation issues with Eigen namespace usage, updating to C++17 standard, and adding a CI workflow for automated testing.
Changes:
- Refactored
load_from_zipto extract zip contents to a temporary directory instead of direct memory mapping - Added test fixtures that programmatically create TRX files for testing instead of relying on external test data
- Updated CMake configuration to require C++17, make tests optional, and properly handle MIO header includes
- Added GitHub Actions CI workflow for automated testing on Ubuntu
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_trx_mmap.cpp | Adds test fixture infrastructure for creating TRX files programmatically and updates existing tests to use the fixture instead of external data files |
| tests/CMakeLists.txt | Updates CMake minimum version to 3.10, changes C++ standard to 17, makes tests optional, and adds MIO include directory handling |
| src/trx.tpp | Replaces sprintf with string concatenation, fixes Eigen namespace issues, improves error messages, refactors load_from_zip to use extraction, replaces canonicalize_file_name with realpath, and adds validation checks |
| src/trx.h | Adds missing includes (limits.h, stdlib.h, stdexcept) and declares new utility functions for temporary directory management and zip extraction |
| src/trx.cpp | Adds type name mappings for long long types, improves error handling in load_header, adds empty memmap support, implements make_temp_dir and extract_zip_to_directory functions, and fixes zip compression settings |
| CMakeLists.txt | Updates CMake minimum version to 3.10, changes C++ standard to 17, and adds MIO include directory handling |
| .github/workflows/trx-cpp-tests.yml | Adds CI workflow for building and testing on Ubuntu with all required dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
If you are feeling pretty good about this, I'd say merge sooner rather than later, because this is clearly already in a better state than what we currently have on main. I added you as a collaborator, so you can merge this when you feel ready. |
|
I added in a conan build like in #5 and addressed the review comments - I think this is good to go! |
2 participants
Hi! I'm hoping to add a TRX reader/writer to ITK and found some issues here. Some of the changes were needed to get it to compile, and a few tests were added. I'm not great at c++, so a detailed review would be appreciated.