Skip to content
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

Add parse_with_rel() for when the rel parameter MUST be present #11

Merged
merged 9 commits into from
Jan 8, 2022

Conversation

mdharm
Copy link
Collaborator

@mdharm mdharm commented Jan 7, 2022

In many situations, it is known beforehand that the Link: header will have a rel parameter. In that case, the HashMap key can be simplified from Option<Rel> to simply Rel.

This applies on top of #10 and resolves #7

mdharm and others added 8 commits January 6, 2022 13:44
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.
Add a new helper function to simplify the returned HashMap for those
cases where we know that the `rel` parameter will be present in the
`Link:` header.

Update tests and documentation as needed.
Somwhere in the merge process, test_error_from() was duplicated.  It was
not flagged as a merge conflict (that I could see), but it needed to be
removed, regardless.
@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2022

Codecov Report

Merging #11 (4b13ef7) into master (12125c4) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #11   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          107       144   +37     
=========================================
+ Hits           107       144   +37     
Impacted Files Coverage Δ
src/lib.rs 100.00% <100.00%> (ø)

@g1eny0ung g1eny0ung self-requested a review January 8, 2022 03:56
Copy link
Owner

@g1eny0ung g1eny0ung left a comment

Choose a reason for hiding this comment

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

LGTM. 🍺

@g1eny0ung g1eny0ung merged commit d5bc871 into g1eny0ung:master Jan 8, 2022
@g1eny0ung g1eny0ung mentioned this pull request Jan 8, 2022
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.

HashMap key is an Option<Rel> even though the relation has been required since 2017
3 participants