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

feat: Enable the Link header #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Oct 4, 2021

Copies over the Link header from the disabled folder and tries to make
the smallest necessary changes to get it working.

Copies over the Link header from the disabled folder and tries to make
the smallest necessary changes to get it working.
@seanmonstar
Copy link
Member

Thanks for the PR! I've purposefully been very careful about what exactly is exposed in the public API, since I learned my lesson before when this crate was part of hyper: too many details for so many types means breaking changes.

In particular, I think the Link header and the things it publicly exposes via LinkValue, are probably too much. I wouldn't want to expose what the data structure is internally, (a Vec<LinkValue>), and I wouldn't want to expose fields of a link-value that are based on types from mime or language_tags. If we could at least make those things just internal, so we can swap them for other things if we want to in the future, then we could move forward with this big typed header.

@pietroalbini
Copy link

Last year I opened #67 to also add typed Link headers, and my PR also fixed other comments @seanmonstar made back then around enums. I don't have the time to push my PR to the finishing line, but feel free to pull changes from my PR :-)

@mfelsche
Copy link

@seanmonstar the missing Link header is the reason why I can't switch from hyperx to headers.
Can you summarize what is wrong with this PR and #67 and how we could move this forward? It would be very much appreciated and I am more than happy to help to get this over the finish line.

@seanmonstar
Copy link
Member

Wow, I'm so sorry, @pietroalbini, that I missed the new changes in #67. It looks pretty much exactly what I've described, when I quickly scroll through. I need to sit down a pay closer attention (there's some new macros :D), but perhaps someone else wants to open a PR with those commits, and we can make the final few edits?

@pietroalbini
Copy link

Wow, I'm so sorry, pietroalbini, that I missed the new changes in #67.

No worries @seanmonstar! I might have 500+ unread notifications myself...

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.

4 participants