Improve `error-handling` by using `error-stack` crate about websurfx HOT 9 CLOSED

So I think we should go with the solution you suggested 🙂, What are your thoughts

Okay, I will implement this solution 🚀 🙂

from websurfx.

@xffxff as we had discussed this issue earlier, would you like to work on this issue?? 🙂

from websurfx.

Yes, I would be willing to work on this issue. @neon-mmd

from websurfx.

I have found that it is not possible to convert scraper::error::SelectorErrorKind to Report because Report requires the error to be Sync and Send. Unfortunately, scraper::error::SelectorErrorKind is not thread-safe, and therefore the following code is invalid:

let no_result: Selector = Selector::parse(".no-results").into_report()?;

from websurfx.

I have found that it is not possible to convert scraper::error::SelectorErrorKind to Report because Report requires the error to be Sync and Send. Unfortunately, scraper::error::SelectorErrorKind is not thread-safe, and therefore the following code is invalid:

let no_result: Selector = Selector::parse(".no-results").into_report()?;

This seems interesting 🙂, Is there any other way to fix this if possible ?? or any other possible solutions?? 🙂 .

from websurfx.

@neon-mmd I think one solution is to avoid converting scraper::error::SelectorErrorKind to Report. Instead, we can create a Report with UnexpectedError

let no_result: Selector = Selector::parse(".no-results")
    .map_err(|_| Report::new(EngineError::UnexpectedError))
    .attach_printable_lazy(|| format!("invalid CSS selector: {}", ".no-results"))?;

However, by doing this, we lose the information about the lower-level source of UnexpectedError, which in this case is scraper::error::SelectorErrorKind. This is not equivalent to our current implementation:

pub enum EngineErrorKind {
    RequestError(reqwest::Error),
    EmptyResultSet,
    UnexpectedError {
        message: String,
        source: Option<Box<dyn std::error::Error>>,
    },
}

from websurfx.

@neon-mmd I think one solution is to avoid converting scraper::error::SelectorErrorKind to Report. Instead, we can create a Report with UnexpectedError

let no_result: Selector = Selector::parse(".no-results")
    .map_err(|_| Report::new(EngineError::UnexpectedError))
    .attach_printable_lazy(|| format!("invalid CSS selector: {}", ".no-results"))?;

However, by doing this, we lose the information about the lower-level source of UnexpectedError, which in this case is scraper::error::SelectorErrorKind. This is not equivalent to our current implementation:

pub enum EngineErrorKind {
    RequestError(reqwest::Error),
    EmptyResultSet,
    UnexpectedError {
        message: String,
        source: Option<Box<dyn std::error::Error>>,
    },
}

I think maybe we could try using the std feature for error-stack, I feel it would solve the issue and if that doesn't work then we will go with this implementation you mentioned. what do you say @xffxff ?? 🙂

from websurfx.

I think maybe we could try using the std feature for error-stack, I feel it would solve the issue

@neon-mmd I think I have already used the std feature, as it is the default feature flag for error-stack.

from websurfx.

I think maybe we could try using the std feature for error-stack, I feel it would solve the issue

@neon-mmd I think I have already used the std feature, as it is the default feature flag for error-stack.

My bad 😄, I think I didn't read this part because the docs say it is built mostly on no-std and the features section mentioned the std feature explicitly, So now we have only one option to forth we would need to make tradeoff making the selector-error less informative while making others more informative and I think this tradeoff won't be that bad I feel because it would make most of the errors more informative than the current implementation which doesn't offer that much information on errors. So I think we should go with the solution you suggested 🙂, What are your thoughts @xffxff ??

from websurfx.