feat(headers): support known extensions #1376
Conversation
|
@seanmonstar: could you let me know when you plan to review this? |
|
This would be a breaking change, right? If so can you add |
|
Sorry, yes, this would be a breaking change, so it couldn't be part of 0.11. I'll put it in the 0.12 milestone, but if someone needs it immediately, one could just copy the code into a local module and use it instead of importing from hyper. |
And only known extensions. Clean up parsing of extensions in general. BREAKING_CHANGE: breaks most uses of `CacheDirective::Extension`.
|
@LegNeato, @seanmonstar: done. @seanmonstar: okay great, but when is a review planned, if any? |
seanmonstar
left a comment
seanmonstar
left a comment
There was a problem hiding this comment.
Thanks! The code looks correct! Unfortunately due to the original design of having public fields and an open enum, these are breaking changes.
If you want, we could also include a redesign of this header to make adding new extensions backwards compatible. If you want to do this, I can write out some changes I'd make on a high level...
| /// Extension directives. Optionally include an argument. | ||
| Extension(String, Option<String>) | ||
| /// Extension directives. | ||
| Extension(CacheDirectiveExtension), |
There was a problem hiding this comment.
The original goal of the Extension variant was to allow for new arguments even if the version of hyper doesn't support them (hence being just Strings). Would it make sense for these new variants to just be on CacheDirective itself?
|
As noted in #189, headers are actually going to moved out of hyper proper. So as to not keep this dangling, I'm going to close this. Thanks for the submission, and of course, because the header system works for anything that implements |
|
@seanmonstar: would it be okay to merge this PR, for the time being? Nobody is going to look up a stale PR branch to reuse any of this work, so it’d be a waste. Even if you plan to release the redesigned |
|
Is there any particular reason you need to pull the header from the Hyper crate specifically? |
|
No there isn’t. I was under the impression that the new headers crate will not be API-compatible with the old module. Apart from that, I assumed that the starting point in terms of headers to reimplement would be the original Hyper |
|
Unfortunately, it can't just be merged, as it's a breaking change. |
4 participants
And only known extensions. Clean up parsing of extensions in general.