Merge "failed" status into "error" status (#191)

I think that the separation between Status::Failed and Status::Error is a
bit misleading. It was easier to implement, but from a user's perspective
they are more or less the same: something unexpected happened.
So I merged both into one: Status::Error. Still not 100% happy with the
semantics, but it's an improvement I'd say.

Cargo.toml

@@ -26,14 +26,14 @@ hubcaps = { git="https://github.com/softprops/hubcaps.git" }
 linkify = "0.5.0"
 regex = "1.4.5"
 url = "2.2.1"
-# Switch back to version on crates.io after
-# https://github.com/async-email/async-smtp/pull/36
-# is merged and a new version of check-if-email-exists is released
+# Switch to version on crates.io after resolving
+# https://github.com/async-email/async-smtp/issues/40 and new version of
+# check-if-email-exists is released
 check-if-email-exists = { git="https://github.com/reacherhq/check-if-email-exists.git" }
 indicatif = "0.15.0"
 structopt = "0.3.21"
 toml = "0.5.8"
-serde = { version = "1.0.123", features = ["derive"] }
+serde = { version = "1.0.124", features = ["derive"] }
 pulldown-cmark = "0.8.0"
 html5ever = "0.25.1"
 markup5ever = "0.10.0"
@@ -50,7 +50,7 @@ serde_json = "1.0.64"
 # See https://github.com/briansmith/ring/issues/1163
 # This is necessary for the homebrew build
 # https://github.com/Homebrew/homebrew-core/pull/70216
-ring = "0.16.19"
+ring = "0.16.20"
 pad = "0.1.6"
 console = "0.14.1"
 fast_chemail = "0.9.6"

README.md

@@ -148,7 +148,7 @@ token with no extra permissions is enough to be able to check public repos links
 There is an extensive list of commandline parameters to customize the behavior,
 see below for a full list.
 
-```
+```ignore
 USAGE:
     lychee [FLAGS] [OPTIONS] [--] [inputs]...
 

src/bin/lychee/stats.rs

@@ -17,9 +17,8 @@ pub fn color_response(response: &Response) -> String {
         Status::Ok(_) => style(response).green().bright(),
         Status::Redirected(_) => style(response),
         Status::Excluded => style(response).dim(),
-        Status::Error(_) => style(response).yellow().bright(),
         Status::Timeout(_) => style(response).yellow().bright(),
-        Status::Failed(_) => style(response).red().bright(),
+        Status::Error(_, _) => style(response).red().bright(),
     };
     out.to_string()
 }
@@ -54,17 +53,16 @@ impl ResponseStats {
     pub fn add(&mut self, response: Response) {
         self.total += 1;
         match response.status {
-            Status::Failed(_) => self.failures += 1,
+            Status::Error(_, _) => self.failures += 1,
             Status::Timeout(_) => self.timeouts += 1,
             Status::Redirected(_) => self.redirects += 1,
             Status::Excluded => self.excludes += 1,
-            Status::Error(_) => self.errors += 1,
             _ => self.successful += 1,
         }
 
         if matches!(
             response.status,
-            Status::Failed(_) | Status::Timeout(_) | Status::Redirected(_) | Status::Error(_)
+            Status::Error(_, _) | Status::Timeout(_) | Status::Redirected(_)
         ) {
             let fail = self.fail_map.entry(response.source.clone()).or_default();
             fail.insert(response);
@@ -152,7 +150,7 @@ mod test_super {
         });
         stats.add(Response {
             uri: website("http://example.org/failed"),
-            status: Status::Failed(http::StatusCode::BAD_GATEWAY),
+            status: Status::Error("".to_string(), Some(http::StatusCode::BAD_GATEWAY)),
             source: Input::Stdin,
         });
         stats.add(Response {
@@ -166,7 +164,7 @@ mod test_super {
             vec![
                 Response {
                     uri: website("http://example.org/failed"),
-                    status: Status::Failed(http::StatusCode::BAD_GATEWAY),
+                    status: Status::Error("".to_string(), Some(http::StatusCode::BAD_GATEWAY)),
                     source: Input::Stdin,
                 },
                 Response {

src/client.rs

@@ -177,7 +177,7 @@ impl Client {
                 // TODO: We should not be using a HTTP status code for mail
                 match self.valid_mail(&address).await {
                     true => Status::Ok(http::StatusCode::OK),
-                    false => Status::Error(format!("Invalid mail address: {}", address)),
+                    false => Status::Error(format!("Invalid mail address: {}", address), None),
                 }
             }
         };
@@ -216,7 +216,7 @@ impl Client {
             Some(github) => {
                 let repo = github.repo(owner, repo).get().await;
                 match repo {
-                    Err(e) => Status::Error(format!("{}", e)),
+                    Err(e) => Status::Error(e.to_string(), None),
                     Ok(_) => Status::Ok(http::StatusCode::OK),
                 }
             }
@@ -224,6 +224,7 @@ impl Client {
                 "GitHub token not specified. To check GitHub links reliably, \
                 use `--github-token` flag / `GITHUB_TOKEN` env var."
                     .to_string(),
+                None,
             ),
         }
     }
@@ -275,7 +276,6 @@ pub async fn check<T: TryInto<Request>>(request: T) -> Result<Response> {
 #[cfg(test)]
 mod test {
     use super::*;
-    use http::StatusCode;
     use std::time::{Duration, Instant};
     use wiremock::matchers::method;
     use wiremock::{Mock, MockServer, ResponseTemplate};
@@ -295,7 +295,18 @@ mod test {
             .check(mock_server.uri())
             .await
             .unwrap();
-        assert!(matches!(res.status, Status::Failed(_)));
+        assert!(res.status.is_failure());
+    }
+
+    #[tokio::test]
+    async fn test_nonexistent_with_path() {
+        let res = ClientBuilder::default()
+            .build()
+            .unwrap()
+            .check("http://127.0.0.1/invalid")
+            .await
+            .unwrap();
+        assert!(res.status.is_failure());
     }
 
     #[tokio::test]
@@ -316,7 +327,7 @@ mod test {
             .unwrap();
         let end = start.elapsed();
 
-        assert!(matches!(res.status, Status::Failed(_)));
+        assert!(matches!(res.status, Status::Error(_, _)));
 
         // on slow connections, this might take a bit longer than nominal backed-off timeout (7 secs)
         assert!(end.as_secs() >= 7);
@@ -336,16 +347,14 @@ mod test {
     }
     #[tokio::test]
     async fn test_github() {
-        assert!(matches!(
-            ClientBuilder::default()
-                .build()
-                .unwrap()
-                .check("https://github.com/lycheeverse/lychee")
-                .await
-                .unwrap()
-                .status,
-            Status::Ok(_)
-        ));
+        assert!(ClientBuilder::default()
+            .build()
+            .unwrap()
+            .check("https://github.com/lycheeverse/lychee")
+            .await
+            .unwrap()
+            .status
+            .is_success());
     }
 
     #[tokio::test]
@@ -357,7 +366,7 @@ mod test {
             .await
             .unwrap()
             .status;
-        assert!(matches!(res, Status::Error(_)));
+        assert!(res.is_failure());
     }
 
     #[tokio::test]
@@ -376,7 +385,7 @@ mod test {
             .await
             .unwrap()
             .status;
-        assert!(matches!(res, Status::Ok(_)));
+        assert!(res.is_success());
     }
 
     #[tokio::test]
@@ -387,7 +396,7 @@ mod test {
             .check("https://expired.badssl.com/")
             .await
             .unwrap();
-        assert!(matches!(res.status, Status::Error(_)));
+        assert!(res.status.is_failure());
 
         // Same, but ignore certificate error
         let res = ClientBuilder::default()
@@ -397,7 +406,7 @@ mod test {
             .check("https://expired.badssl.com/")
             .await
             .unwrap();
-        assert!(matches!(res.status, Status::Ok(_)));
+        assert!(res.status.is_success());
     }
 
     #[tokio::test]
@@ -408,7 +417,7 @@ mod test {
             .check("https://crates.io/crates/lychee")
             .await
             .unwrap();
-        assert!(matches!(res.status, Status::Failed(StatusCode::NOT_FOUND)));
+        assert!(res.status.is_failure());
 
         // Try again, but with a custom header.
         // For example, crates.io requires a custom accept header.
@@ -422,7 +431,7 @@ mod test {
             .check("https://crates.io/crates/lychee")
             .await
             .unwrap();
-        assert!(matches!(res.status, Status::Ok(_)));
+        assert!(res.status.is_success());
     }
 
     #[tokio::test]

src/types.rs

@@ -78,11 +78,17 @@ impl Response {
 impl Display for Response {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         let metadata = match &self.status {
-            Status::Ok(code) | Status::Redirected(code) | Status::Failed(code) => {
+            Status::Ok(code) | Status::Redirected(code) => {
                 format!(" [{}]", code)
             }
             Status::Timeout(code) if code.is_some() => format!(" [{}]", code.unwrap()),
-            Status::Error(e) => format!(" ({})", e),
+            Status::Error(e, code) => {
+                if let Some(code) = code {
+                    format!(" ({})", code)
+                } else {
+                    format!(" ({})", e)
+                }
+            }
             _ => "".to_string(),
         };
         write!(f, "{} {}{}", self.status.icon(), self.uri, metadata)
@@ -95,15 +101,13 @@ pub enum Status {
     /// Request was successful
     Ok(http::StatusCode),
     /// Request failed with HTTP error code
-    Failed(http::StatusCode),
+    Error(String, Option<http::StatusCode>),
     /// Request timed out
     Timeout(Option<http::StatusCode>),
     /// Got redirected to different resource
     Redirected(http::StatusCode),
     /// Resource was excluded from checking
     Excluded,
-    /// Low-level error while loading resource
-    Error(String),
 }
 
 impl Display for Status {
@@ -112,8 +116,13 @@ impl Display for Status {
             Status::Ok(c) => format!("OK ({})", c),
             Status::Redirected(c) => format!("Redirect ({})", c),
             Status::Excluded => "Excluded".to_string(),
-            Status::Failed(c) => format!("Failed ({})", c),
-            Status::Error(e) => format!("Runtime error ({})", e),
+            Status::Error(err, code) => {
+                if let Some(code) = code {
+                    format!("Failed: {} ({})", err, code)
+                } else {
+                    format!("Failed: {}", err)
+                }
+            }
             Status::Timeout(Some(c)) => format!("Timeout ({})", c),
             Status::Timeout(None) => "Timeout".to_string(),
         };
@@ -139,7 +148,7 @@ impl Status {
         } else if statuscode.is_redirection() {
             Status::Redirected(statuscode)
         } else {
-            Status::Failed(statuscode)
+            Status::Error("".into(), Some(statuscode))
         }
     }
 
@@ -147,6 +156,10 @@ impl Status {
         matches!(self, Status::Ok(_))
     }
 
+    pub fn is_failure(&self) -> bool {
+        matches!(self, Status::Error(_, _))
+    }
+
     pub fn is_excluded(&self) -> bool {
         matches!(self, Status::Excluded)
     }
@@ -156,8 +169,7 @@ impl Status {
             Status::Ok(_) => "✔",
             Status::Redirected(_) => "⇄️",
             Status::Excluded => "?",
-            Status::Failed(_) => "✗",
-            Status::Error(_) => "↯",
+            Status::Error(_, _) => "✗",
             Status::Timeout(_) => "⧖",
         }
     }
@@ -168,7 +180,7 @@ impl From<reqwest::Error> for Status {
         if e.is_timeout() {
             Status::Timeout(e.status())
         } else {
-            Status::Error(e.to_string())
+            Status::Error(e.to_string(), e.status())
         }
     }
 }

tests/usage.rs

@@ -41,13 +41,13 @@ mod readme {
             .expect("Invalid utf8 output for `--help`");
         let readme = load_readme_text();
 
-        const BACKTICKS_OFFSET: usize = 3; // marker: ```
+        const BACKTICKS_OFFSET: usize = 9; // marker: ```ignore
         const NEWLINE_OFFSET: usize = 1;
 
         let usage_start = BACKTICKS_OFFSET
             + NEWLINE_OFFSET
             + readme
-                .find("```\nUSAGE:\n")
+                .find("```ignore\nUSAGE:\n")
                 .expect("Couldn't find USAGE section in README.md");
 
         let usage_end = readme[usage_start..]