Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to a concrete Error type #156

Closed
wants to merge 1 commit into from
Closed

Switch to a concrete Error type #156

wants to merge 1 commit into from

Conversation

belak
Copy link

@belak belak commented Mar 26, 2020

Please note that this is specifically a PoC to replace usage of Box<dyn std::error::Error + Send + Sync + 'static> with a concrete surf::Error type.

There are a few things yet to figure out:

  • Documentation for surf::{Result, Error}
  • Whether or not the Error variants should be exposed to the end user or if they should be hidden. I went with hiding it because it's similar to anyhow::Error.
  • When to return a surf::Error vs alternatives (like serde_json::Error) or if surf::Error should always be used
  • How errors should be displayed. Should they be wrapped or displayed as-is?

The main Error implementation is in error.rs and is also exported at the root of the crate.

This is an alternative to simply re-exporting anyhow::Error and would be another method of fixing #86.

@zkat
Copy link
Collaborator

zkat commented Jul 14, 2020

hey @Fishrock123 @yoshuawuyts can we take this one? I think this would solve the error issue nicely.

@belak I notice you have a lot of conflicts. Are you ok with fixing them, or would you rather someone else do it, at this point?

@Fishrock123
Copy link
Member

At a glance this sounds right to me, that’s what Tide does.

@belak
Copy link
Author

belak commented Jul 14, 2020

I'd be happy to work on this, but it might take a few days since I've got a bunch of other stuff going on

Copy link
Member

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Ok so I wrote that and these comments without 100% realizing it until after, but what I said here actually happened already in c8366fb. I thought something was a little off.

(Also @zkat ^)

So... this isn't necessary anymore. Thanks anyways though.

one part hasn't been 100% completed though, if you'd like to pick it up still: convert all Result<T, Err> to surf::Result<T>.

#[allow(missing_docs)]
#[derive(ThisError, Debug)]
#[error(transparent)]
pub struct Error(#[from] InternalError);
Copy link
Member

Choose a reason for hiding this comment

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

This probably doesn't need to exist. Ideally this would just using http_types::Error instead: https://docs.rs/http-types/2.3.0/http_types/struct.Error.html

(It's implemented like this: https://github.com/http-rs/http-types/blob/master/src/error.rs)

impl_from!(serde_json::Error);

#[derive(ThisError, Debug)]
pub(crate) enum InternalError {
Copy link
Member

Choose a reason for hiding this comment

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

Also with http_types::Error this shouldn't be necessary, because it wraps the errors and allows you to check downcasts, like so:

match surf::get(url).await {
    Ok() -> (),
    Err(error) -> {
        if let Some(&url_err) = error.downcast_ref::<UrlError>() {
            // ...
        } else if let Some(&json_err) = error.downcast_ref::<JsonError>() {
            // ...
        }
    }
}

use thiserror::Error as ThisError;

#[allow(missing_docs)]
pub type Result<T> = StdResult<T, Error>;
Copy link
Member

Choose a reason for hiding this comment

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

This already exists from the recent-ish ac215cb

@Fishrock123
Copy link
Member

Closing since as noted above, the better change would have been a switch to http_types::Error, which was already done in April in c8366fb!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants