Support generic error types #5
Conversation
src/cache_api.rs
Outdated
| #[error("An error occurred when trying to submit the cache request")] | ||
| TokioMpscSendError(), | ||
| #[error("An error occurred when trying to join the result future")] | ||
| FutureJoinError(tokio::task::JoinError), |
There was a problem hiding this comment.
Consider using #[from] for each of these to automatically generate Into implementations:
| FutureJoinError(tokio::task::JoinError), | |
| FutureJoinError(#[from] tokio::task::JoinError), |
There was a problem hiding this comment.
Your suggested change doesn't compile.
I'm not quite sure, but it seems the derive generates some code which clashes with the code generated from the #[from].
I'll check if its a simple thing, or if I just implement the Into traits myself.
There was a problem hiding this comment.
Interesting- if you post the compilation error, I can suggest what may be breaking.
It's also possible that the issue is because thiserror struggles with Generic Parameters: dtolnay/thiserror#79
| #[derive(Error, Debug)] | ||
| pub enum CacheLoadingError<E: Debug> { | ||
| #[error("An error occurred when trying to submit the cache request")] | ||
| TokioMpscSendError(), |
There was a problem hiding this comment.
Potentially, tokio errors may be worth grouping together. Do you intend for the user to ever "handle" one of these?
I'd consider a second error type, CacheCommunicationsError or similar, which then contains details on whether it was a Send issue, a failure to join futures, etc.
Generally, I'd expect the user is to be interested in: LoadingError, LookupLoop, and NoData (Is this one actually expected to be seen by the user at all, or is it internally used to trigger a load?). Beyond that- everything else strikes me as "I probably just want to panic if it's one of these".
There was a problem hiding this comment.
This one indeed makes sense. I think the only error actually relevant to the user is the LoadingError.
In any other case something is odd with the cache.
LookupLoop happens when the cache state doesnt mutate at all, and it would have to do the same lookup again.
NoData is when you call an update function, but the loader doesn't return success, then there's "NoData" to update/mutate.
The most frequent error case will be the LoadingError though, eventually NoData will be interesting too.
The others are actually really edge cases.
There was a problem hiding this comment.
Implemented in new commit, thanks for the suggestion!
| pub loading_error: Option<LoadingError>, | ||
| // todo: nested errors | ||
| #[derive(Error, Debug)] | ||
| pub enum CacheLoadingError<E: Debug> { |
There was a problem hiding this comment.
Copying from #3 for contextual proximity:
Potentially, a few helpers for the
CacheLoadingErrortype may be warranted, such asinto_loading_error(self) -> Option<E>/as_loading_error(&self) -> Option<&E>helpers.
There was a problem hiding this comment.
I think I implemented these utility functions in my new commit
Move internal stuff into CacheCommunicationError
Wow, they don't clash because there's no generics in here, nice
Co-authored-by: Zoey <Dessix@Dessix.net>
As wished in #3 here's an implementation for generic error types