fix: Fix error handling in write_repodata by removing anyhow::Error and adding a dedicated error type #1899
Conversation
…avoid anyhow::Error usage
crates/rattler_index/Cargo.toml
Outdated
| path = "src/main.rs" | ||
|
|
||
| [dependencies] | ||
| thiserror = "1.0" |
There was a problem hiding this comment.
use workspace dependencies
There was a problem hiding this comment.
Updated — changed this error to use workspace dependency. Thanks for pointing this out.
crates/rattler_index/src/error.rs
Outdated
|
|
||
| #[derive(Debug, Error)] | ||
| pub enum RepodataError { | ||
| #[error("I/O error: {0}")] |
There was a problem hiding this comment.
Lets make this transparent
crates/rattler_index/src/error.rs
Outdated
| #[error("I/O error: {0}")] | ||
| Io(#[from] io::Error), | ||
|
|
||
| #[error("Serialization error: {0}")] |
| Patch(String), | ||
|
|
||
| #[error("Opendal error: {0}")] | ||
| Opendal(#[from] OpendalError), |
crates/rattler_index/src/error.rs
Outdated
| #[error("Opendal error: {0}")] | ||
| Opendal(#[from] OpendalError), | ||
|
|
||
| #[error("Other error: {0}")] |
crates/rattler_index/src/lib.rs
Outdated
| ) | ||
| .await?; | ||
| .await | ||
| .map_err(RepodataError::Opendal)?; |
There was a problem hiding this comment.
Since from is implemented, this is not needed
crates/rattler_index/src/lib.rs
Outdated
| Some(CACHE_CONTROL_REPODATA), | ||
| ) | ||
| .await?; | ||
| .await |
crates/rattler_index/src/lib.rs
Outdated
| Some(CACHE_CONTROL_REPODATA), | ||
| ) | ||
| .await?; | ||
| .await |
There was a problem hiding this comment.
@baszalmstra I have updated the PR based on your feedback. Here is a summary of the changes:
Custom Error Type: Replaced anyhow::Result with a dedicated RepodataError enum in src/error.rs to allow for structured error handling.
Module Visibility: Added pub mod error; with proper documentation in src/lib.rs.
Retry Logic: Updated the index_subdir loop to properly match against RepodataError::Opendal when detecting race conditions, ensuring retries still work as intended.
Dependencies: Updated Cargo.toml to use thiserror = { workspace = true } as requested.
I've verified that cargo test, cargo clippy, and cargo doc all pass locally. Ready for another review!
crates/rattler_index/src/lib.rs
Outdated
| let mut hasher = Sha256::new(); | ||
| hasher.update(&encoded); | ||
| let digest = hasher.finalize(); | ||
| Ok((k.clone(), (digest, encoded))) |
DCchoudhury15
changed the title
|
i have done all the changes and also resolved my errors up for review. |
|
@baszalmstra please take a look at the pr raised for this issue. |
2 participants
This PR resolves Issue #1726 by introducing a structured RepodataError type and removing the use of anyhow::Error inside write_repodata and related indexing code.
Changes include:
anyhow::Errorusages with RepodataError.thiserror.This makes the error surface more predictable and fixes failing error conversions in rattler_index.