Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions src/imageproxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,33 +30,33 @@ use tracing::instrument;
#[derive(Error, Debug)]
#[non_exhaustive]
pub enum Error {
#[error("i/o error")]
#[error("i/o error: {0}")]
/// An input/output error
Io(#[from] std::io::Error),
#[error("skopeo spawn error: {}", .0)]
#[error("skopeo spawn error: {0}")]
/// An error spawning skopeo
SkopeoSpawnError(#[source] std::io::Error),
#[error("serialization error")]
#[error("serialization error: {0}")]
/// Returned when serialization or deserialization fails
SerDe(#[from] serde_json::Error),
/// The proxy failed to initiate a request
#[error("failed to invoke method {method}: {error}")]
RequestInitiationFailure { method: Box<str>, error: Box<str> },
/// An error returned from the remote proxy
#[error("proxy request returned error")]
#[error("proxy request returned error: {0}")]
RequestReturned(Box<str>),
#[error("semantic version error")]
#[error("semantic version error: {0}")]
SemanticVersion(#[from] semver::Error),
#[error("proxy too old (requested={requested_version} found={found_version}) error")]
/// The proxy doesn't support the requested semantic version
ProxyTooOld {
requested_version: Box<str>,
found_version: Box<str>,
},
#[error("configuration error")]
#[error("configuration error: {0}")]
/// Conflicting or missing configuration
Configuration(Box<str>),
#[error("error")]
#[error("other error: {0}")]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For a general catch-all variant like Other, prefixing the error message with "other error: " can be redundant, as the variant name itself already provides this context. Displaying the wrapped error message directly often leads to cleaner and more precise error reporting.

Suggested change
#[error("other error: {0}")]
#[error("{0}")]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm I would rather explicitly know that the error came from the Other variant just so it's easier to know where it might be coming from.

/// An unknown other error
Other(Box<str>),
}
Expand All @@ -72,9 +72,9 @@ impl Error {
#[non_exhaustive]
pub enum GetBlobError {
/// A client may reasonably retry on this type of error.
#[error("retryable error")]
#[error("retryable error: {0}")]
Retryable(Box<str>),
#[error("error")]
#[error("other error: {0}")]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the Error::Other variant, prefixing the message with "other error: " is redundant for GetBlobError::Other. Displaying the contained message directly would be cleaner and more direct.

Suggested change
#[error("other error: {0}")]
#[error("{0}")]

/// An unknown other error
Other(Box<str>),
}
Expand Down