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

Builders don't have a fn to set the header map when the user already has a HeaderMap value. #91

Open
carllerche opened this issue Aug 2, 2017 · 9 comments
Labels
A-request Area: Request A-response Area: Response B-rfc Blocked: request for comments. More discussion would help move this along. S-feature Severity: feature. This adds something new.

Comments

@carllerche
Copy link
Collaborator

Currently, the only builder fns are for inserting header entries, but if you just want to set the header map, there is no API for that.

@carllerche carllerche added this to the 0.1 milestone Aug 2, 2017
@carllerche
Copy link
Collaborator Author

It is currently possible to set the HeaderMap after building:

let mut request = Request::builder()
    .method("get")
    .body(());

*request.headers_mut() = my_headers;

This isn't super ergonomic or discoverable.

This issue is flagged as 0.1 as it might require breaking changes if Builder::headers is renamed.

@alexcrichton
Copy link
Contributor

We could change headers?

@carllerche
Copy link
Collaborator Author

We could... the extend behavior also seems useful to have as well, but I couldn't think of a great name.

@alexcrichton
Copy link
Contributor

Maybe something like headers_reset?

@carllerche
Copy link
Collaborator Author

I guess replace_headers could work... It is unfortunate that all the other setters explicitly set the value except the one for HeaderMap.

The other option would be to rename the current headers fn -> insert_headers.

Also, a thought occurs to me. Currently, headers takes an into iterator of (K, V) which has the possibility of having duplicate keys. HeaderMap has two ways to handle duplicate keys (replace or append). Maybe having insert_headers and append_headers on the builder isn't too terrible?

@seanmonstar
Copy link
Member

Something that occurs to me is it doesn't sound like anyone is super convinced by any of the names. It's easy to add in methods later, but hard to fix these after they are added.

@carllerche
Copy link
Collaborator Author

@seanmonstar I'm totally fine punting on adding fns. The real question here is what to do about the headers fn that is currently part of the builder.

Maybe we should just yank it for 0.1 and defer figuring out what that fn should do.

@seanmonstar
Copy link
Member

If we aren't certain that headers should extend (append), then seems prudent to remove it for now. 👍

carllerche added a commit that referenced this issue Aug 7, 2017
As discussed in #91, the exact signature & behavior of `headers` is
uncertain. In order to be conservative, this PR removes those functions
until we can figure out what it should do exactly.
@carllerche
Copy link
Collaborator Author

After removing the headers fn, this issue can be fixed post 0.1.

@carllerche carllerche removed this from the 0.1 milestone Aug 12, 2017
@seanmonstar seanmonstar added S-feature Severity: feature. This adds something new. B-rfc Blocked: request for comments. More discussion would help move this along. A-request Area: Request A-response Area: Response labels Oct 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-request Area: Request A-response Area: Response B-rfc Blocked: request for comments. More discussion would help move this along. S-feature Severity: feature. This adds something new.
Projects
None yet
Development

No branches or pull requests

3 participants