fix: Improve output formatting

With the last lychee release, we simplified the status output for links. While this reduced the visual noise, it also accidentally caused the source of errors to not be printed anymore. This change brings back the additional error information as part of the final report output. Furthermore, it shows the error information in the progress output if verbose mode is activated. Fixes #1487
lycheeverse · Nov 6, 2024 · ebe82c8 · ebe82c8
1 parent 1b6675a
commit ebe82c8
Show file tree

Hide file tree

Showing 7 changed files with 92 additions and 17 deletions.
diff --git a/fixtures/TEST_DETAILED_JSON_OUTPUT_ERROR.md b/fixtures/TEST_DETAILED_JSON_OUTPUT_ERROR.md
@@ -1,8 +1,8 @@
-# Test detailed JSON output error
+# Test Detailed JSON Output Error
 
-This file is used to test if the error details are parsed properly in the json
+This file is used to test if the error details are parsed properly in the JSON
 format.
 
-[The website](https://expired.badssl.com/) produce SSL expired certificate
-error. Such network error has no status code but it can be identified by error
-status details.
+[The website](https://expired.badssl.com/) produces an SSL expired certificate
+error. Such a network error has no status code, but it can be identified by
+error status details.
diff --git a/fixtures/TEST_INVALID_URLS.html b/fixtures/TEST_INVALID_URLS.html
@@ -0,0 +1,27 @@
+<!DOCTYPE html>
+<html lang="en">
+  <head>
+    <meta charset="UTF-8" />
+    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
+    <title>Invalid URLs</title>
+  </head>
+  <body>
+    <ul>
+      <li>
+        <a href="https://httpbin.org/status/404"
+          >https://httpbin.org/status/404</a
+        >
+      </li>
+      <li>
+        <a href="https://httpbin.org/status/500"
+          >https://httpbin.org/status/500</a
+        >
+      </li>
+      <li>
+        <a href="https://httpbin.org/status/502"
+          >https://httpbin.org/status/502</a
+        >
+      </li>
+    </ul>
+  </body>
+</html>
diff --git a/lychee-bin/src/formatters/response/plain.rs b/lychee-bin/src/formatters/response/plain.rs
@@ -61,7 +61,7 @@ mod plain_tests {
         let body = mock_response_body(Status::Excluded, "https://example.com/not-checked");
         assert_eq!(
             formatter.format_response(&body),
-            "[EXCLUDED] https://example.com/not-checked | Excluded"
+            "[EXCLUDED] https://example.com/not-checked"
         );
     }
 

diff --git a/lychee-bin/src/formatters/stats/compact.rs b/lychee-bin/src/formatters/stats/compact.rs
@@ -42,8 +42,7 @@ impl Display for CompactResponseStats {
             for response in responses {
                 writeln!(
                     f,
-                    "[{}] {}",
-                    response.status.code_as_string(),
+                    "{}",
                     response_formatter.format_detailed_response(response)
                 )?;
             }

diff --git a/lychee-bin/tests/cli.rs b/lychee-bin/tests/cli.rs
@@ -111,6 +111,50 @@ mod cli {
         }};
     }
 
+    /// Test that the default report output format (compact) and mode (color)
+    /// prints the failed URLs as well as their status codes on error. Make
+    /// sure that the status code only occurs once.
+    #[test]
+    fn test_compact_output_format_contains_status() -> Result<()> {
+        let test_path = fixtures_path().join("TEST_INVALID_URLS.html");
+
+        let mut cmd = main_command();
+        cmd.arg("--format")
+            .arg("compact")
+            .arg("--mode")
+            .arg("color")
+            .arg(test_path)
+            .env("FORCE_COLOR", "1")
+            .assert()
+            .failure()
+            .code(2);
+
+        let output = cmd.output()?;
+
+        // Check that the output contains the status code (once) and the URL
+        let output_str = String::from_utf8_lossy(&output.stdout);
+
+        // The expected output is as follows:
+        // "Find details below."
+        // [EMPTY LINE]
+        // [path/to/file]:
+        //      [400] https://httpbin.org/status/404
+        //      [500] https://httpbin.org/status/500
+        //      [502] https://httpbin.org/status/502
+        // (the order of the URLs may vary)
+
+        // Check that the output contains the file path
+        assert!(output_str.contains("TEST_INVALID_URLS.html"));
+
+        let re = Regex::new(r"\s{5}\[\d{3}\] https://httpbin\.org/status/\d{3}").unwrap();
+        let matches: Vec<&str> = re.find_iter(&output_str).map(|m| m.as_str()).collect();
+
+        // Check that the status code occurs only once
+        assert_eq!(matches.len(), 3);
+
+        Ok(())
+    }
+
     /// JSON-formatted output should always be valid JSON.
     /// Additional hints and error messages should be printed to `stderr`.
     /// See https://github.com/lycheeverse/lychee/issues/1355
@@ -996,7 +1040,7 @@ mod cli {
             .stderr(contains(format!(
                 "[IGNORED] {unsupported_url} | Unsupported: Error creating request client"
             )))
-            .stderr(contains(format!("[EXCLUDED] {excluded_url} | Excluded\n")));
+            .stderr(contains(format!("[EXCLUDED] {excluded_url}\n")));
 
         // The cache file should be empty, because the only checked URL is
         // unsupported and we don't want to cache that. It might be supported in

diff --git a/lychee-lib/src/types/response.rs b/lychee-lib/src/types/response.rs
@@ -79,19 +79,24 @@ pub struct ResponseBody {
 // matching in these cases.
 impl Display for ResponseBody {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        // Always write the URI
         write!(f, "{}", self.uri)?;
 
-        if let Status::Ok(StatusCode::OK) = self.status {
-            // Don't print anything else if the status code is 200.
-            // The output gets too verbose then.
+        // Early return for OK status to avoid verbose output
+        if matches!(self.status, Status::Ok(StatusCode::OK)) {
             return Ok(());
         }
 
-        // Add a separator between the URI and the additional details below.
-        // Note: To make the links clickable in some terminals,
-        // we add a space before the separator.
-        write!(f, " | {}", self.status)?;
+        // Format status and return early if empty
+        let status_output = self.status.to_string();
+        if status_output.is_empty() {
+            return Ok(());
+        }
+
+        // Write status with separator
+        write!(f, " | {}", status_output)?;
 
+        // Add details if available
         if let Some(details) = self.status.details() {
             write!(f, ": {details}")
         } else {

diff --git a/lychee-lib/src/types/status.rs b/lychee-lib/src/types/status.rs
@@ -48,12 +48,12 @@ impl Display for Status {
             Status::Ok(code) => write!(f, "{code}"),
             Status::Redirected(code) => write!(f, "Redirect ({code})"),
             Status::UnknownStatusCode(code) => write!(f, "Unknown status ({code})"),
-            Status::Excluded => f.write_str("Excluded"),
             Status::Timeout(Some(code)) => write!(f, "Timeout ({code})"),
             Status::Timeout(None) => f.write_str("Timeout"),
             Status::Unsupported(e) => write!(f, "Unsupported: {e}"),
             Status::Error(e) => write!(f, "{e}"),
             Status::Cached(status) => write!(f, "{status}"),
+            Status::Excluded => Ok(()),
         }
     }
 }