Repository navigation
Split up and refactor Status enum #2097
Description
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)
Activity
mre commentedon Mar 25, 2026
Yes, I fully agree. The status enum does too much.
Minor remark: I like the term
Outcomebut what do you think aboutVerdictinstead ofConclusion? Then it would readVerdict::Ok,Verdict::Error. Alternatively,Resolution?katrinafyi commentedon Mar 25, 2026
Verdict sounds good to me! I don't like Resolution so much because we already have quite a few resolvers.
The verdict enum could even inherit the name Status, after it's been slimmed down. I'm not too fussy about the names.
mre commentedon Mar 25, 2026
Ah, I haven't thought of that. My vote goes to that. Good idea. 👍
mre commentedon Apr 4, 2026
I just now discover how much your work will contribute to recursion support as well, which is awesome!
If this refactor lands, recursion gets structured body access without bolting anything onto the existing
Responsetype.Instead, the
Outcome::Websitevariant could naturally carry the response body:This way, recursion gets body access "for free." I.e. we don't need a new field bolted onto the Response type (so no API pollution because the new design accommodates it). The
collect_responseshandler would pattern-match onOutcome::Website, extract the body, and feed it to theExtractor. This is the cleanest option for recursion support as far as I can see, so I'd very much like to see this landing soon.mre commentedon Apr 4, 2026
One more observation is that
ExcludedandUnsupportedare not on the same "logical level" as the other variants inOutcome.They're not really "outcomes of a check" like the rest but rather reasons a check didn't happen. We could model them separately from
Outcome:The downside is that we'd have to go through one more level of indirection when we want to get to the outcome, but that might even be a good thing because it could simplify some of the status logic.
mre commentedon Apr 4, 2026
Outcome::Cachedrequires some more thought.A cache hit means we didn't perform a check. We retrieved a previous result/outcome instead. The cached value is itself an
Outcome(or at least a lossy summary of one).If the cache stored
(Outcome, Status)pairs instead, a cache hit would return these directly, never constructing a new Outcome. This meansOutcomedoesn't need aCachedvariant at all.I believe this is cleaner conceptually (caching is orthogonal to what happened), but it requires rethinking how the cache integrates. The current proposal keeps the current cache architecture mostly intact, so we could keep it for now, but I still wanted to put that up for discussion.
mre commentedon Apr 4, 2026
We might want
UnsupportedandUnknownas separate variants insideStatus:Currently:
is_unsupported()is used inignore_cache()(incheck.rs) and in formatters (dimmed color)is_unknown()is used inignore_cache()(yellow color, separate counter)unsupportedandunknownas distinct bucketsIf we fold these into
ExcludedorError, we lose the ability to display them distinctly.katrinafyi commentedon Apr 7, 2026
I think I agree with all of your recent messages (sorry for the late reply).
My plan was that the *Checker types would return Outcome. Since excluding happens before the checker, the exclude case shouldn't be in there. With caching, we could add that as a case into your CheckResult. Since we don't cache errors anymore, it is very simple and that case shouldn't need any extra data at all.
Alternatively, it is interesting to use a "lossy summary" of Outcome as a cache value, but I think that's not necessary unless we want to start storing more information in the cache. For example, we might store whether the cache entry was created with include-fragments. But I think this is further down the line and we just keep it simple for now.
Finally, about the Status enum, yeah, it was always my intention that this enum line up with whatever counters we want to show. Hopefully, removing all the complex Status cases makes this much easier to manage in future.
mre commentedon Apr 7, 2026
Cool, okay.
I like that!
Would you like to post an updated version of the types?