Error Handling #10

Open sylv.gay
Open opened by sylv.gay

This is probably something more suited for 0.16.0, but the current method of handling errors is quite hacky. Although convoluted, applications can simply wrap around the returned anyhow::Result or use anyhow::error::Error::downcast_ref to unwrap specific errors.

However, in the case of XRPC errors, the error response is decoded using error.to_string().contains("ErrorName") on the reqwest::error::Error. For example, atpxrpc uses this trick to determine if an error is ExpiredToken. This could be fixed by using something similar to XrpcResponse in atproto_client::com::atproto::* when sending XRPC requests to ATProto servers.

In general, it would be best to use a crate like thiserror to more clearly specify in the API what errors functions can produce. That way, you can create a type alias like XrpcResult<T> = Result<T, XrpcError> (may need a wrapper using thiserror instead of just XrpcError) which shows API users a standard set of errors. I'm not entirely sure if XrpcError is useful here though since unknown errors wouldn't be good for API. Something more generic like a struct that only contains code and message would make more sense IMO.

Yeah, that's a good call-out. The way anyhow is used comes from when it was more deeply used in the application https://smokesignal.events/. I should have done a deeper refactor at the time to move over to thiserror following the guidance that anyhow was for apps and thiserror is for libraries.

I'm going to spend some time this week looking at this sort of refactor. I think you're right that it might be a 0.16.0 scale thing, but like, 0.15.0-rc* is so massively different than 0.14 that it might be worth it to just dive into that change.

Sign up or Sign in to add to the discussion.