-
Notifications
You must be signed in to change notification settings - Fork 6
Add additional error handling for crypto.rs and accompany tests #97
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
Conversation
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.
Pull Request Overview
This PR renames the InvalidFilepath error to InvalidFileOrDirPath across modules, updates relevant context calls in file-open and directory-read paths, and adds a new test for nested directory hashing to increase coverage.
- Renamed error variant and updated
contextuses incrypto.rsand Docker orchestrator. - Updated
Debugimplementation to reference the new error variant. - Added
nested_dir_hashtest intests/crypto.rsto cover recursive directory hashing.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/crypto.rs | Imported new UDL model types, extended lint allowances, and added nested_dir_hash test |
| src/uniffi/orchestrator/docker.rs | Updated context call to use InvalidFileOrDirPath error variant |
| src/uniffi/error.rs | Renamed InvalidFilepath variant to InvalidFileOrDirPath |
| src/core/error.rs | Updated Debug match arm to use the new variant name |
| src/core/crypto.rs | Updated hash_file and hash_dir to use InvalidFileOrDirPath error context |
Comments suppressed due to low confidence (1)
src/core/crypto.rs:65
- Consider adding a test that exercises the new error handling branch in
hash_dirby passing a non-existent or inaccessible directory path, to verify thatInvalidFileOrDirPathis returned as expected.
.read_dir()
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
src/core/store/filestore.rs
Outdated
| serde_yaml::from_str( | ||
| &fs::read_to_string(path.clone()) | ||
| .context(selector::InvalidFilepath { path })?, | ||
| .context(selector::InvalidFileOrDirPath { path })?, |
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.
Why not just call this InvalidPath?
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.
True that is a better name, can issue the change in a bit
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.
Do you want me to merge as is and you change it in other PR or make changes in this PR?
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.
I an update it then you can merge it
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.
Updated
Fixes PLT-234