Skip to content

Split up and refactor Status enum #2097

@katrinafyi

Description

@katrinafyi

I feel like the Status enum has grown beyond its original purpose and has become too big and complicated.

The main problem is that it mixes up two things which I think should be separated: (1) the actual outcome of the request, and (2) how that outcome should be interpreted. Another problem is that the enum has grown data fields in the form of the Redirected variant. This history information should really be separate from the actual status and stored in a struct.

The presence of functions like Status::is_success, is_error, etc shows that the shape of the enum does not match the shape of the information you want it to store - ideally, these would all be match statements on a flat enum type. Functions like is_success_ignoring_timeouts are also desiring more flexibility than the current types can provide.

I've sketched out a rough idea for splitting it up into two parts:

  • Outcome, the objective outcome of the check, and
  • Status, the interpretation of the Outcome after taking into account --accept, etc.
/// Outcome of the checker, without any presumption of success or failure.
pub enum Outcome {
    Website {
        result: Result<StatusCode, reqwest::Error>,
        redirects: Redirects,
        fragment_check: Option<bool>,
    },
    Mail {
        result: mailify_lib::CheckResult,
    },
    File {
        result: Result<Metadata, io::Error>,
        applied_fallback_extension: Option<String>,
        applied_index_file: Option<String>,
        fragment_check: Option<bool>,
    },
    RequestError(RequestError),
    Unsupported(ErrorKind),
}

/// A check which was performed or skipped. This includes excluded and cached,
/// unlike Outcome which is for checks which actually run.
pub enum CheckResult {
    Checked(Outcome),   // we actually performed a check
    Skipped(SkipReason), // Excluded, Unsupported, etc.
    CachedSuccess, // since recently, we only cache successes
}

/// Response to a lychee Request. Bundles a CheckResult along with metadata
/// (like source location) and pre-check transformations like remaps.
pub struct Response {
    check_result: CheckResult,
    request: Request, // <-- remap_history, span is inside Request
    input_source: crate::InputSource,
    response_body: crate::ResponseBody,
}

/// An interpretation of an [`Outcome`], taking into account user-provided
/// configuration. Corresponds to counters in the CLI output.
pub enum Status {
    Ok,
    Error,
    Timeout,
    Redirected,
    Cached,
    Excluded,
    Unsupported,
    Unknown,
}

impl Status {
    /// For example.
    pub fn from_response(
        response: Response,
        accept: StatusCodeSelector,
        accept_timeouts: bool,
        other_things: MoreOptions,
    ) -> Self {
        Self::Ok
    }
}

Obviously this is all open to change. Let me know what you think.

Here's a comment where I mentioned this before: #2063 (comment)

Metadata

Metadata

Assignees

No one assigned

    Type

    No fields configured for Task.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions