-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement feature to use url::Url
rather than http::Uri
#10
Conversation
Per the crate regex documentation, compiling a regular expression is computationally expensive. Thus, it is advantageous when processing multiple Link: headers to use lazy_static! to compile it only once. Also add some comments to struct Link for better doc rendering. Add test cases to achieve 100% code coverage, per tarpaulin.
Implement a feature named "url" which swaps the http::Uri out for a url::Url -- this allows direct use with other popular crates, such as reqwest. Currently, crate url is used by over 2.5k other crates, whereas crate http is only used by about 1.2k other crates (less than half as many). Since many "Link:" headers come from web servers which are giving a relationship between the current page and other pages, very often the URI is actually a URL, and this is known ahead of time. Generally, features which cause semver breaking changes are frowned upon, but there is really no other way that I can see -- implementing traits like From<&Uri> for Url is forbidden. Without this feature, the only other way is to use the raw_uri with Url::parse(), effectively using String as an intermediate format to translate between the two types. Since URLs can't be relative, support for relative refs is lost when this feature is enabled. Documentation (in-code and README.md) as well as test cases are also updated. Tarpaulin reports 100% coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Agree with u. I have no problem with adding the |
I'm confused. You say that you agree, and you have approved the review, and all checks have passed -- but this PR has not yet been merged. Is there something else you need? |
No 🤣, just some other things at the time, and then forgot, I will merge this PR now. |
This adds feature "url", which swaps the Uri for an URL. This is useful, as many other packages use URLs rather than URIs, and it is often known ahead of time that the
Link
header contains a URL.This resolves #8 and must be applied on top of #9