Remove the Redirect and Remapped wrapper variants from the Status enum.

[mre]

The goal of this refactor is to make a step towards cleaning up the Status enum as proposed in #2097.

Previously, redirect and remap information was nested inside the Status enum, which forced every exhaustive match (~8 places) to either recurse through these wrappers or delegate via the innermost() helper. This made the enum harder to work with and obscured the actual status.

The proposal is to have redirect and remap data live as separate fields on ResponseBody.
This gets rid of innermost() and redirects() methods from Status and all is_success/is_error/etc. methods now match directly on self.

I am aware that the three Nones in a row in Response are pretty bad. The constructor already had too many positional params before this change, and I made it worse.

However, this change will unlock the possibility to move redirects into Outcome in the future, e.g.

pub enum Outcome {
    Website {
        result: Result<StatusCode, reqwest::Error>,
        redirects: Redirects,
        fragment_check: Option<bool>,
    },
    // ...
}

This way, we can get rid of the None fields in Response again in the future when we introduce Outcome.

(I attempted that first, but it turned out to be a much bigger refactor, so I broke out a smaller chunk for now.)

[katrinafyi]

Very happy with the refactor and the resulting simplifications. Looks good!

It's not specific to this PR, but I was never clear on the separation between Response and ResponseBody.

In #2097, it was also my intention to store Remap inside Request and then store Request inside Response. With that, we would avoid needing to move all the fields across into Response(Body), like span, source, etc. Nothing needs to change in this PR, but just checking we're on the same page :)

[mre]

In #2097, it was also my intention to store Remap inside Request and then store Request inside Response. With that, we would avoid needing to move all the fields across into Response(Body), like span, source, etc. Nothing needs to change in this PR, but just checking we're on the same page :)

Sounds like a plan. No objections right now. My only concern is that Response becomes quite a big type, so printing might get harder, but inspection should get easier. So it's a tradeoff.

I've never been a huge fan of the standalone ResponseBody anyway. Well, having a strong type instead of Option<String> would be nice, but the fact that ResponseBody was completely detached from Response caused some headaches in the past, like the field-copying/alignment that you mentioned.

[katrinafyi]

Yeah, the main use of ResponseBody is it appears in the response maps:

    pub(crate) success_map: HashMap<InputSource, HashSet<ResponseBody>>,

where ResponseBody is basically a Response minus the InputSource.

pub struct Response {
    input_source: InputSource,
    response_body: ResponseBody,
}

This makes some sense, as the hashmap already has the input source in the key. But yeah, just something to keep in mind for later.

[thomas-zahner]

Thank you @mre, I have no objections. I agree that we can clean up the Nones with a new PR as you explained.

Sorry for introducing this confusing approach in the first place. Additionally, the current nesting made it possible to introduce useless and confusing state such as containing Redirected or Remapped multiple times. This is something I didn't think about when I introduced it. So your refactor is definitely the way to go 👍

[mre]

Cool! I will rebase the PR and apply the changes we agreed on when I get the chance. Will merge when everything is clean.

[mre]

Just some more lints to fix. 😅 @thomas-zahner, guess you're on it?

[mre]

Thanks for pushing this over the finishing line, Thomas!