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 back the typed representation of the Link header #113

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Jun 29, 2022

This is exactly #67 except it uses the latest version of language-tags and is rebased.

edit: Now one extra commit of my own, as commit authorship shows.

Closes #67.

@jplatte
Copy link
Contributor Author

jplatte commented Sep 21, 2022

ping @seanmonstar

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

They're not part of the diff, so I can't comment at the specific line, but I think a few parts of the public API should be adjusted, so they make less promises about internals. Such as:

  • Link::new(Vec<_>) - Perhaps it should be new(), taking no arguments, to start with an empty link header.
  • Link::values() -> &[LinkValue] - This implies we have a contiguous slice of values, but we could conceivably place them in a different data structure (a vecdeque, or a hashmap), or even eagerly serialize them. I'd probably change this to return an impl Iterator. Maybe some of the other types name this as iter? I can't recall.

@jplatte
Copy link
Contributor Author

jplatte commented Oct 20, 2022

Sounds good! I added a FromIterator implementation as a less "biased" version of the previous new, so it's still convenient to construct a Link from a list of LinkValues.

I found no other occurrences of a values method but many iters, so I updated that name.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Besides the couple of comments which should be quick to fix, a final check would be good: is there any type, or method, which is exposing it's internal structure? Any forwards-compatibility hazards that if we needed to change, would need a breaking change?

self.values.as_ref()
/// Get an iterator over the `Link` header's `LinkValue`s.
pub fn iter(&self) -> impl Iterator<Item = LinkValue> + '_ {
self.values.iter().cloned()
Copy link
Member

Choose a reason for hiding this comment

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

Should the default iterator be cloning the values? Why not give references, and a user can clone if they need to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe so the internal storage can be changed to something that isn't a container of LinkValues? Happy to change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want it changed?

@@ -25,6 +25,7 @@ bytes = "1"
mime = "0.3.14"
sha1 = "0.10"
httpdate = "1"
language-tags = "0.3.2"
Copy link
Member

Choose a reason for hiding this comment

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

I've tried scrolling through to find the answer, but failed so far. We don't expose this dependency, right?

Copy link
Contributor Author

@jplatte jplatte Feb 27, 2023

Choose a reason for hiding this comment

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

We are, actually. LinkValue::href_lang returns Option<&[LanguageTag]>, and LinkValue::push_href_lang has the parameter language_tag: LanguageTag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you want that to change before merging?

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather not expose this dependency publicly, at least that's my default feeling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I feel like it shouldn't be such a big deal for headers, as opposed to headers-core. But I'll look into how complex it would be to remove it from public API / what the consequences would be exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the main reason I would like to not change the code here is that it allows push_href_lang to be infallible. If language-tags was required to be removed from public API, that would mean parsing the language tag provided by the user in that function. Please let me know whether you agree or not and I'll update the PR accordingly.

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