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 other::Expect header #266

Merged
merged 1 commit into from
Nov 27, 2020
Merged

Add other::Expect header #266

merged 1 commit into from
Nov 27, 2020

Conversation

yoshuawuyts
Copy link
Member

Add other::Expect header. Ref #99. Thanks!

@Fishrock123
Copy link
Member

Is 100-continue the only relevant value here?

@yoshuawuyts
Copy link
Member Author

@Fishrock123 yep, and it seems unlikely any others will be added anytime soon. No known browser currently uses this mechanism; it seems curl is the primary user of this right now.

@Fishrock123
Copy link
Member

Should we even implement this? Does Curl require it or something? i.e. is it useful?

@jbr
Copy link
Member

jbr commented Nov 2, 2020

We use it in async-h1 server, and when async-h1 client behavior is slightly more mature it should also use this header. I don't really see much value in including it as a reusable type, though. It's just as easy to do .insert_header("Expect", "100-continue"). We're trying to wrap up headers like there's a finite set of them, but there will always be headers that need to be set with string values

@yoshuawuyts
Copy link
Member Author

@jbr I mostly included it for completion. We also have typed headers for e.g. Content-Length which is arguably trivial too.

@Fishrock123
Copy link
Member

Parsing Content-Length is not exactly uncommon though.

@yoshuawuyts
Copy link
Member Author

It seems the conversation now is about useful this header will turn out to be, rather than whether the implementation is correct. I don't see any concrete downsides in including this; the worst case seems to be that nobody will care. With the upside that this might actually be helpful to some.

Making a decision to merge this.

@yoshuawuyts yoshuawuyts merged commit 975ad8f into main Nov 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the expect branch November 27, 2020 14:18
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