[hermes] Add shadow copy creation #3006
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 @@
## Gb7f1abf97eed8a035dbcaf7b0363b5890baaec05 #3006 +/- ##
==========================================================================
Coverage 91.87% 91.87%
==========================================================================
Files 20 20
Lines 6057 6057
==========================================================================
Hits 5565 5565
Misses 492 492 ☔ 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 the capability to create a shadow copy of a project for the hermes verification tool. It adds the walkdir dependency to traverse the file system and includes a new shadow module with logic for creating a skeleton of the source project using symlinks. The changes also include a refactoring of how compilation roots are resolved and a new check to ensure all local dependencies are contained within the workspace. The code is well-structured and the error handling is robust. My feedback is focused on minor improvements to error message formatting for better user experience.
| anyhow::bail!( | ||
| "Unsupported external dependency: '{}' at {:?}.\n\ | ||
| Hermes currently only supports verifying workspaces where all local \ | ||
| dependencies are contained within the workspace root.", | ||
| pkg.name, | ||
| pkg_path | ||
| ); |
There was a problem hiding this comment.
For more user-friendly error messages, it's better to use .display() for printing paths instead of the Debug format ({:?}). The Debug format for Path includes quotes, which can make the output less clean for users.
| anyhow::bail!( | |
| "Unsupported external dependency: '{}' at {:?}.\n\ | |
| Hermes currently only supports verifying workspaces where all local \ | |
| dependencies are contained within the workspace root.", | |
| pkg.name, | |
| pkg_path | |
| ); | |
| anyhow::bail!( | |
| "Unsupported external dependency: '{}' at {}.\n\ | |
| Hermes currently only supports verifying workspaces where all local \ | |
| dependencies are contained within the workspace root.", | |
| pkg.name, | |
| pkg_path.display() | |
| ); |
|
|
||
| if entry.file_type().is_dir() { | ||
| fs::create_dir_all(&dest_path) | ||
| .with_context(|| format!("Failed to create shadow directory: {:?}", dest_path))?; |
There was a problem hiding this comment.
To improve the readability of error messages, consider using .display() for formatting paths instead of the Debug format ({:?}). The Debug format adds quotes around the path, which is often not ideal for user-facing output.
| .with_context(|| format!("Failed to create shadow directory: {:?}", dest_path))?; | |
| .with_context(|| format!("Failed to create shadow directory: {}", dest_path.display()))?; |
| #[cfg(unix)] | ||
| fn make_symlink(original: &Path, link: &Path) -> Result<()> { | ||
| std::os::unix::fs::symlink(original, link) | ||
| .with_context(|| format!("Failed to symlink {:?} -> {:?}", original, link)) |
There was a problem hiding this comment.
For cleaner and more user-friendly error messages, it's preferable to use .display() to format paths instead of the Debug format ({:?}).
| .with_context(|| format!("Failed to symlink {:?} -> {:?}", original, link)) | |
| .with_context(|| format!("Failed to symlink {} -> {}", original.display(), link.display())) |
| // Since we only call this on files (checking is_file above), | ||
| // we use symlink_file. | ||
| std::os::windows::fs::symlink_file(original, link) | ||
| .with_context(|| format!("Failed to symlink {:?} -> {:?}", original, link)) |
There was a problem hiding this comment.
To improve the readability of error messages, consider using .display() for formatting paths instead of the Debug format ({:?}). This will provide a cleaner output for the user.
| .with_context(|| format!("Failed to symlink {:?} -> {:?}", original, link)) | |
| .with_context(|| format!("Failed to symlink {} -> {}", original.display(), link.display())) |
joshlf
force-pushed
the
Gdc72294eb3c40c4afa61ea6e8f965ba310cd22ef
branch
from
169b4eb to
c475f79
Compare
joshlf
force-pushed
the
Gb7f1abf97eed8a035dbcaf7b0363b5890baaec05
branch
from
7c251d8 to
3d5982e
Compare
gherrit-pr-id: Gdc72294eb3c40c4afa61ea6e8f965ba310cd22ef
joshlf
force-pushed
the
Gb7f1abf97eed8a035dbcaf7b0363b5890baaec05
branch
from
3d5982e to
bbe5419
Compare
joshlf
force-pushed
the
Gdc72294eb3c40c4afa61ea6e8f965ba310cd22ef
branch
from
c475f79 to
bb0da31
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/Gdc72294eb3c40c4afa61ea6e8f965ba310cd22ef && git checkout -b pr-Gdc72294eb3c40c4afa61ea6e8f965ba310cd22ef FETCH_HEADCheckout
git fetch origin refs/heads/Gdc72294eb3c40c4afa61ea6e8f965ba310cd22ef && git checkout FETCH_HEADCherry Pick
git fetch origin refs/heads/Gdc72294eb3c40c4afa61ea6e8f965ba310cd22ef && git cherry-pick FETCH_HEADPull
Stacked PRs enabled by GHerrit.