-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
(feat): Add request.set_headers and request.prepend_headers #53
Conversation
Thank you for this, but seeing as one can pipe the existing function I don't yet know of a strong case for their additions. What was the situation which prompted you to implement these? |
I like constructing headers in their own function, generally, into a list and adding them to requests as such. Here's an example fn to_http_request(
client: Client,
method: Method,
req: Request,
) -> HttpRequest(String) {
let headers =
client
|> headers
|> merge_headers(req.headers)
let body =
req.body
|> option.map(json.to_string)
|> option.unwrap("")
let query =
option.map(req.query, fn(params) {
list.map(params, fn(param) {
let key = pair.first(param)
let value = pair.second(param)
#(uri.percent_encode(key), uri.percent_encode(value))
})
})
|> option.unwrap([])
request.new()
|> request.set_method(method)
|> request_ext.set_headers(headers)
|> request.set_scheme(Https)
|> request.set_host(host)
|> request.set_body(body)
|> request.set_path(req.path)
|> request.set_query(query)
} Full code: https://github.com/dmmulroy/glitch/blob/main/src/glitch/api/client.gleam#L97 |
I'd also add there is kind of a precedence for this type of API in the library already with pub fn set_query(
req: Request(a),
query: List(#(String, String)),
) -> Request(a) |
In this specific case I think it'd be best to use the Gleam HTTP request type rather than having a custom one |
I don't think I'm fully following what you're suggesting here - could you clarify? I concur that I definitely don't need the extra
Request(..request.new(), headers: headers)
|> request.set_method(method)
|> request.set_scheme(Https)
|> request.set_host(host)
|> request.set_body(body)
|> request.set_path(req.path)
|> request.set_query(query)
|
Let's keep this being something in your codebase for now, and we can see if it's something that there's a demand for later. Time often brings more clarity. |
Sounds good - thanks for engaging and thanks for your time, always appreciate it. |
No, thank you!! Contributions are super important |
Okay I'm back, and I feel more strongly now that this should be added. I just ran into a really confusing "bug" where I had a http request (using |> request.get_header("content-type") It does not find the header because I add my headers dynamically and as a list. Right now, the obvious way to do this, as we discussed above, is to supply the header list to the constructor like this2: Request(..request.new(), method: Get, headers: headers) Since using a "builder" pattern is fairly idiomatic in Gleam, and appears to be the preferred api with I am also opening a second PR3 that I think is more important than this one to make Footnotes |
See #54 for additional context on why this was closed |
This PR adds pluralized versions for
set_header
andprepend_header
that will each take a list of#(String, String)
and apply them to the request in the same fashion as the singular versions. In addition, tests have been added to cover the new functions.