[hermes] Add package/target/root resolution #3005
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## G9611b9a3ca0254d8ba488d591e6a21980cca3d29 #3005 +/- ##
============================================================================
Coverage ? 91.87%
============================================================================
Files ? 20
Lines ? 6057
Branches ? 0
============================================================================
Hits ? 5565
Misses ? 492
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces package, target, and root resolution for hermes by integrating clap for command-line argument parsing and cargo_metadata for inspecting the project structure. The new resolve module contains the core logic for identifying which parts of a crate or workspace to process based on user-provided flags, mimicking cargo's behavior. The main function is updated to use this new resolution mechanism.
The changes are well-structured. I've identified a few areas for improvement regarding error handling and code cleanliness, including a potential panic. My detailed comments are below.
| let manifest_dir = p.manifest_path.parent().unwrap(); | ||
| cwd.starts_with(manifest_dir) |
There was a problem hiding this comment.
Using .unwrap() here could cause a panic if a Cargo.toml file is located at the root of the filesystem, as parent() would return None. While this is an edge case, it's safer to handle it gracefully. You can use an if let statement for a more robust implementation.
| let manifest_dir = p.manifest_path.parent().unwrap(); | |
| cwd.starts_with(manifest_dir) | |
| if let Some(manifest_dir) = p.manifest_path.parent() { | |
| cwd.starts_with(manifest_dir) | |
| } else { | |
| false | |
| } |
|
|
||
| let file_path = PathBuf::from(&args[1]); | ||
| // TODO: Better error handling than `.unwrap()`. | ||
| let roots = resolve::resolve_roots(&args.resolve).unwrap(); |
There was a problem hiding this comment.
As noted in the TODO comment, using .unwrap() here can cause the program to panic if resolve_roots returns an error. It would be more user-friendly to handle the error gracefully and print a descriptive message instead of panicking. Since anyhow is a dependency, you can use unwrap_or_else to achieve this.
| let roots = resolve::resolve_roots(&args.resolve).unwrap(); | |
| let roots = resolve::resolve_roots(&args.resolve).unwrap_or_else(|e| { | |
| eprintln!("error: failed to resolve verification roots: {e:?}"); | |
| exit(1); | |
| }); |
| eprint!("{:?}", miette::Report::new(e)); | ||
| } else if let Ok(item) = res { | ||
| transform::append_edits(&item, &mut edits); | ||
| for (package, kind, path) in roots { |
There was a problem hiding this comment.
The package and kind variables are unused within this loop. To improve code clarity and avoid compiler warnings, you can replace them with _.
| for (package, kind, path) in roots { | |
| for (_, _, path) in roots { |
joshlf
force-pushed
the
G9611b9a3ca0254d8ba488d591e6a21980cca3d29
branch
from
d92de78 to
777a0e4
Compare
joshlf
force-pushed
the
Gb7f1abf97eed8a035dbcaf7b0363b5890baaec05
branch
from
7c251d8 to
3d5982e
Compare
gherrit-pr-id: Gb7f1abf97eed8a035dbcaf7b0363b5890baaec05
joshlf
force-pushed
the
Gb7f1abf97eed8a035dbcaf7b0363b5890baaec05
branch
from
3d5982e to
bbe5419
Compare
joshlf
force-pushed
the
G9611b9a3ca0254d8ba488d591e6a21980cca3d29
branch
from
777a0e4 to
3150404
Compare
2 participants
mod foo;declarations #3008Latest Update: v34 — Compare vs v33
📚 Full Patch History
Links show the diff between the row version and the column version.
⬇️ Download this PR
Branch
git fetch origin refs/heads/Gb7f1abf97eed8a035dbcaf7b0363b5890baaec05 && git checkout -b pr-Gb7f1abf97eed8a035dbcaf7b0363b5890baaec05 FETCH_HEADCheckout
git fetch origin refs/heads/Gb7f1abf97eed8a035dbcaf7b0363b5890baaec05 && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/Gb7f1abf97eed8a035dbcaf7b0363b5890baaec05 && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.