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)
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 likeis_success_ignoring_timeoutsare also desiring more flexibility than the current types can provide.I've sketched out a rough idea for splitting it up into two parts:
Obviously this is all open to change. Let me know what you think.
Here's a comment where I mentioned this before: #2063 (comment)