fix: graceful error handling for grit check in uninitialized contexts
#61
Conversation
WalkthroughThe recent update streamlines the error handling within the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/gritcache/src/new_cache.rs (2 hunks)
Additional comments: 1
crates/gritcache/src/new_cache.rs (1)
- 182-184: The addition of this test case effectively validates the improved error handling behavior when the cache file is missing, and cache refresh is disabled. This is a valuable addition to ensure the robustness of the application.
crates/gritcache/src/new_cache.rs
Outdated
| return Err(e).context(format!( | ||
| "Failed to read cache file {}", | ||
| path.to_string_lossy() | ||
| )); |
There was a problem hiding this comment.
The simplification of error handling by removing specific checks for file existence is a positive change. However, consider adding more context to the error message to inform the user about potential actions, such as running grit init if the cache file does not exist.
- return Err(e).context(format!(
- "Failed to read cache file {}",
- path.to_string_lossy()
- ));
+ return Err(e).context(format!(
+ "Failed to read cache file {}. Please ensure the cache has been initialized by running `grit init`.",
+ path.to_string_lossy()
+ ));Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return Err(e).context(format!( | |
| "Failed to read cache file {}", | |
| path.to_string_lossy() | |
| )); | |
| return Err(e).context(format!( | |
| "Failed to read cache file {}. Please ensure the cache has been initialized by running `grit init`.", | |
| path.to_string_lossy() | |
| )); |
|
playing with this a little more -- the creation logic for the backing file is correct as implemented assuming exists on the FS, so this definitely isn't the right fix. EDIT: setting a would a better approach be to add a check here that the |
|
Yeah, I think we want to do it this way:
Generally we should avoid doing pre-emptive checks but instead handle failures if they come up. I think it might actually be fine to edit this so instead of panicking that thread gracefully terminates with a suggestion to run |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- crates/gritcache/src/new_cache.rs (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- crates/gritcache/src/new_cache.rs
| @@ -39,8 +39,8 @@ impl ThreadedCache { | |||
| }; | |||
|
|
|||
| let (sender, receiver) = mpsc::channel::<HashKey>(); | |||
| let mut writer = Self::new_writer(&mismatches_path)?; | |||
There was a problem hiding this comment.
@morgante let me know if there's a reason not to do this i'm missing - seems more straightforward to bail during creation than to do so from the writer thread
There was a problem hiding this comment.
This looks fine - if the compiler is happy, I'm happy.
mparker3
changed the title
grit check in uninitialized contexts
| @@ -39,8 +39,8 @@ impl ThreadedCache { | |||
| }; | |||
|
|
|||
| let (sender, receiver) = mpsc::channel::<HashKey>(); | |||
| let mut writer = Self::new_writer(&mismatches_path)?; | |||
There was a problem hiding this comment.
This looks fine - if the compiler is happy, I'm happy.
(hopefully!) no functional changes.
calling
grit checkbeforegrit initright now panics:fix results in:
probably not a super common occurence but it was one of the first things I ran into test driving the CLI, can replicate with the added test in the absence of the fix. Happy to take this PR in a different direction if needed
Summary by CodeRabbit