fix: restore request::Error builder errors as Unsupported#1982
Closed
katrinafyi wants to merge 6 commits intolycheeverse:masterfrom
Closed
fix: restore request::Error builder errors as Unsupported#1982katrinafyi wants to merge 6 commits intolycheeverse:masterfrom
katrinafyi wants to merge 6 commits intolycheeverse:masterfrom
Conversation
idk if this was an intentional change in the rate limiting change. the change happened because `enum Status` has special logic for detecting error kinds, but this only happens when `From<reqwest::Error>` is invoked, and not `From<ErrorKind>`. the logic says that some reqwest errors get treated as different status depending on their type. in particular, the data URI case got an Unsupported status. the rate limiting change did an extra .into() (via ?) which converted a reqwest error into an ErrorKind, and this happened _before_ the into Status conversion. ErrorKinds are universally turned into Error statuses along the way, i also turn `TryFrom<&Url> for HostKey` into `From<&Url> for HostKey`, by setting a fallback hostname when the URL has none of its own. this should be conceptually fine, but maybe it will be surprising if it turns up in user-facing output. Fixes https://www.github.com/lycheeverse/lychee/issues/1980
Member
Author
|
it seems like setting a fallback hostname is not the way to go? at least, the tests don't like it. feedback would be appreciated. that said, i do feel like it shouldn't be an error if there's no hostname... not sure |
Member
|
Thank you for the investigation and PR! It wasn't really intentional I think, but to fix some failing tests IIRC. I guess we didn't consider and test URLs which do not contain any hostnames. I will give this some thought. |
Co-authored-by: Thomas Zahner <thomas.zahner@protonmail.ch>
Merged
Member
Author
|
closing in favour of #1990 :) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
idk if this was an intentional change in the rate limiting change. the change happened because
enum Statushas special logic for detecting error kinds, but this only happens whenFrom<reqwest::Error>is invoked, and notFrom<ErrorKind>. the logic says that some reqwest errors get treated as different status depending on their type. in particular, the data URI case got an Unsupported status.the rate limiting change did an extra .into() (via
?) which converted a reqwest error into an ErrorKind, and this happened before the into Status conversion.this PR fixes the bug by changing
From<ErrorKind> for Statusto dispatch toFrom<reqwest::Error> for Statusin the BuildRequestClient case. this allows ErrorKind::BuildRequestClient to also be treated as Unsupportedalong the way, i also turn
TryFrom<&Url> for HostKeyintoFrom<&Url> for HostKey, by setting a fallback hostname when the URL has none of its own. this should be conceptually fine, but maybe it will be surprising if it turns up in user-facing output. this could be worked around another way if desired.Fixes #1980