Skip to content

Conversation

@mre
Copy link
Member

@mre mre commented Jun 16, 2025

Related to #1723

The goal is to get as much information from the error as possible.
This started out as an experiment, but now I think it would be useful to provide better error messages.

The downside is that we'd have to pay close attention to changes in upstream error messages (in hyper/reqwest). I feel like that is a sensible tradeoff to make, given that the error messages rarely change, and we merely check for specific, relatively "robust" keywords.

@mre mre requested a review from thomas-zahner August 18, 2025 14:26
Copy link
Member

@thomas-zahner thomas-zahner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your efforts. This will improve UX quite a lot!

@mre
Copy link
Member Author

mre commented Aug 24, 2025

@thomas-zahner Thanks for the excellent feedback! I've addressed all your comments:

Changes Made:

  1. Added helpful error messages for the three None cases:
    - InvalidFilePath: Now returns "File not found at path: '{uri}'. Check if file exists
    and path is correct"
    - InvalidFragment: Now returns "Fragment not found in document: '{uri}'. Check if
    fragment exists or page structure"
    - InvalidIndexFile: Now returns "Index file not found in directory: '{}'. Check if
    index.html or other index files exist"

  2. Refactored analyze_error_chain to eliminate clippy::too_many_lines:
    - First, I broke the 287-line function into focused helper functions
    - Further evolved it into a clean builder pattern using ErrorRules
    - All manual loops eliminated in favor of declarative rule definitions

Reasoning:

The original approach with individual if contains() chains was repetitive and hard to maintain. I've implemented an ErrorRules builder that encapsulates the pattern matching logic:

  ErrorRules::new()
      .rule(&["expired", "NotValidAtThisTime"], "SSL certificate expired. Site needs to 
  renew certificate")
      .rule(&["hostname", "NotValidForName"], "SSL certificate hostname mismatch. Check 
  URL spelling")
      .fallback("SSL certificate error. Check certificate validity")
      .match_error(error_msg)

This approach provides better maintainability: Rules are declarative, and adding new patterns ist just adding new .rule() calls. Also, each rule set can have custom fallback behavior.

I hope the code now more readable. What do you think?

@mre mre requested a review from thomas-zahner August 25, 2025 09:47
Copy link
Member

@thomas-zahner thomas-zahner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mre That's a really great approach 👍

mre and others added 2 commits August 25, 2025 16:52
@mre mre merged commit 41b7397 into master Aug 25, 2025
6 checks passed
@mre mre deleted the reqwest-err branch August 25, 2025 15:02
@mre
Copy link
Member Author

mre commented Aug 25, 2025

Thanks for the thorough review, @thomas-zahner

This was referenced Aug 25, 2025
This was referenced Oct 21, 2025
@mre mre mentioned this pull request Nov 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants