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

Create a new AcceptEncoding header, and supporting structs/enums #70

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

Conversation

ParkMyCar
Copy link

This PR creates a new version of the AcceptEncoding header which is built on top of a new struct QualityValue and enum ContentCoding

QualityValue is a wrapper around FlatCsv that parses the values in the csv as if they might have a quality value delimited by a default ";q=". We adhere to the spec for quality value, namely, if a value in the header doesn't have a weighting, we assign a default value of 1, and then sort equal values by order in the header (test cases are included that assert this).

ContentCoding is an enum that represents common values passed with Accept-Encoding, notably, "gzip", "deflate", "compress", "br", and "identity". The enum and its impls are derived via a new macro with doc tests to show behavior.

@nicksrandall
Copy link

I'd like to help get this PR approved and merged. Is there anything I can do to help?

@novacrazy
Copy link

Is the itertools dependency strictly necessary? Who is this waiting on for approval? @seanmonstar ?

It's been almost 11 months since this was opened, and seanmonstar/warp#513 is still waiting on this as well, which is a rather important step towards not needing nginx in front of warp.

@joseluisq
Copy link

Any progress on this?

@novacrazy
Copy link

Would be nice to have this merged this year, as it is required for handling compression correctly. I've been using a fork until now with this, but now I want to publish crates relying on the functionality.

What is the reason for this to not be accepted?

@attila-lin
Copy link

Any progress on this?

@RReverser
Copy link

@seanmonstar Any chance to review this? I'm actually interesting in Accept not Accept-Encoding, but this seems to lay a solid foundation for other similar headers as per #7.

@seanmonstar
Copy link
Member

Probably my biggest concern is around the QualityItem type. I feel like it was very clunky back when it existed in hyper::header.

Besides that, the design goal of porting the types in this crate has been to expose as minimal an API surface as possible, especially around internal storage, so that we can make changes for optimization purposes and not need more breaking releases. (So, not showing public fields, not giving out references to fields, but instead returning impl Iterators, etc.)

@RReverser
Copy link

This seems to encapsulate most APIs behind methods pretty well, except for yeah, QualityItem. Is there any better alternative it could be replaced with?

Not having Accept* headers is pretty limiting, and there are already Rust servers that implement their parsing incorrectly as a workaround (e.g. literally doing .split(",") on header value), so at this point even clunky design feels better than nothing.

@novacrazy
Copy link

Version 0.4 of headers released without this. This could have been merged 3 years ago and improved upon since then to fix any emergent issues, but it's just sat here while everyone maintains their own outdated fork to support compression, myself included. Perfection is the enemy of progress.

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.

7 participants