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

Ergonomics of custom headers (header macro) #11

Closed
anowell opened this issue Nov 10, 2016 · 14 comments
Closed

Ergonomics of custom headers (header macro) #11

anowell opened this issue Nov 10, 2016 · 14 comments

Comments

@anowell
Copy link
Contributor

anowell commented Nov 10, 2016

It might be nice to duplicate hyper's header! macro for sending custom headers - for lack of macro_reexport. I moved an API client from hyper to reqwest, and custom headers is now the only reason I directly depend on hyper. Alternatively, splitting out headers (a la hyperium/hyper#894) into hyper-headers or hyper-types also seems like a reasonable way to get the extra macro.

@aleksanb
Copy link

I went for raw_headers as opposed to the typed headers in an app which uses reqwest for now,
couldn't figure out how to merge the output of the header! macro from hyper with reqwest.

I'd be up for implementing this if someone could guide me in the correct direction (Is a copy-paste of the header macro the best solution?).

@retrry
Copy link

retrry commented Jan 25, 2017

This is how I used header! macro with reqwest

header! { (CustomAuthenticationHeader, "custom_auth") => [String] }

let http_client = reqwest::Client::new().unwrap();
let mut response = http_client.post(url)
  .header(CustomAuthenticationHeader(api_key))
  .body(body)
  .send()
  .unwrap();

It still would be nice, that reqwest reexported header! macro, as it is the only thing, why I directly have to depend on hyper.

@seanmonstar
Copy link
Owner

It still would be nice, that reqwest reexported header! macro, as it is the only thing, why I directly have to depend on hyper.

I know! But macro_reexport is an unstable feature, so it cannot be done on stable Rust. 😢

@aleksanb
Copy link

Could we as a stopgap measure til the feature becomes stable, re-implement the macro in the reqwest library?

@seanmonstar
Copy link
Owner

That's a possibility. I wonder if it makes sense to copy verbatim, of if this is an opportunity to make the macro less confusing. Perhaps keywords in the macro instead of sigils would make things more clear?

header! {
    name = "X-Foo";
    type = Foo;
    repr = [String]; // I don't know if Vec<String> would work, but if it would, maybe that's better?
}

@aleksanb
Copy link

I like the key-value style of your suggestion,
although the choice of key "type" for the generated rust struct feels weird,
as it's not a preexisting type but something generated by the macro.

Alternatives quickly get verbose, f.ex with

header! {
    rust_name = Foo;
    serialized_name = "X-Foo";
    repr = [String];
}

@echochamber
Copy link
Contributor

I think serialized name is still a bit foreign. I'd prefer calling it something like http_header_name.

header! {
    rust_name = Foo;
    http_header_name = "X-Foo";
    repr = [String];
}

@softprops
Copy link

are there any examples for how to get a header? reqwest's header map takes a value argument and hypers takes a type argument.

@seanmonstar
Copy link
Owner

@softprops assuming you're asking about current reqwest (0.8.x), you can make use of type inference, or use turbofish syntax:

let ty: ContentType = res.headers().get().unwrap();

let len = res.headers().get::<ContentLength>().unwrap;

@softprops
Copy link

Sry I was mistaken. I was actually using the http crate which I thought reqwest used.

@seanmonstar seanmonstar added this to the 0.9 milestone Jul 4, 2018
@seanmonstar seanmonstar removed this from the 0.9 milestone Aug 23, 2018
@kornelski
Copy link
Contributor

Could you switch to the http create for headers? It has HeaderName::from_str.

The macros are enormous hassle. I also don't see how such type-strict solution could work with header names only known at run time (e.g. Vary and Connection can refer to arbitrary header names).

@seanmonstar
Copy link
Owner

Could you switch to the http create for headers?

The upcoming 0.9 release upgrades to hyper 0.12, which adds support for the http crate. I'm working an upgrade for the typed headers as well, so each can pick how to deal with headers.

@otavio
Copy link

otavio commented Sep 22, 2018

@seanmonstar when doing it, are you also keeping support for the ByteRangeSpec and others?

I ended doing a hackish solution for now, when upgrading to 0.9 release, but it is far from neat... :-(

repi pushed a commit to EmbarkStudios/reqwest that referenced this issue Dec 20, 2019
repi pushed a commit to EmbarkStudios/reqwest that referenced this issue Dec 20, 2019
…-dsn

Add constructor for SentryCredential from DSN
@seanmonstar
Copy link
Owner

reqwest no longer has typed headers built in (could conceivably come back some day).

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

No branches or pull requests

8 participants