Use strongly typed errors based on thiserror #134
Conversation
Dyn<error> is generally discouraged in libraries.
|
|
||
| ### Changed | ||
|
|
||
| - Error hierarchy revamp to structured errors based on thiserror crate |
There was a problem hiding this comment.
| - Error hierarchy revamp to structured errors based on thiserror crate | |
| - Error hierarchy revamp to structured errors based on `thiserror` crate |
There was a problem hiding this comment.
Nit: maybe link to it too thiserror ?
| #[derive(Debug, Clone)] | ||
| pub struct ResponseContent<T> { | ||
| pub struct ResponseContent<T: std::fmt::Debug> { |
There was a problem hiding this comment.
Nit / style:
I think the preferred way would be to
use std::fmt::Debug;
and then:
| pub struct ResponseContent<T: std::fmt::Debug> { | |
| pub struct ResponseContent<T: Debug> { |
Applied to all instances of std::fmt::Debug.
And then the same would apply for Display trait below at line 8.
| #[error("reqwest error: {0}")] | ||
| Reqwest(#[from] reqwest::Error), |
There was a problem hiding this comment.
I wonder if it isn't better to use #[error(transparent)] attribute on these as we are basically just passing them over to the caller.
| #[error("reqwest error: {0}")] | |
| Reqwest(#[from] reqwest::Error), | |
| #[error(transparent)] | |
| Reqwest(#[from] reqwest::Error), |
I guess it would make sense to use the current form if we intended to add some more details to the error.
| @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| ## [0.3.4] - 2024-08-27 | |||
There was a problem hiding this comment.
Given that 0.4.0 went in but hasn't been released yet, should we include these changes in the same version too?
| @@ -39,7 +39,7 @@ impl FalconHandle { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| pub async fn from_env() -> Result<Self, Box<dyn std::error::Error>> { | |||
| pub async fn from_env() -> Result<Self, crate::error::CredentialsError> { | |||
There was a problem hiding this comment.
| pub async fn from_env() -> Result<Self, crate::error::CredentialsError> { | |
| pub async fn from_env() -> Result<Self, CredentialsError> { |
Dyn is generally discouraged in libraries.
Reraise of #131 from a branch instead of main in my fork. It's easier to keep fork:main in sync and rebase feature branch as upstream progresses.