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

Allow any Associative as a header #132

Closed
wants to merge 2 commits into from

Conversation

omus
Copy link
Contributor

@omus omus commented Dec 4, 2017

Allows alternative Dict types such as PyCall.PyDict to be used as a header.

@omus
Copy link
Contributor Author

omus commented Dec 6, 2017

I could add some tests explicitly for this but most likely I would need to add a test requirement on DataStructures.jl to use an OrderedDict

@quinnj
Copy link
Member

quinnj commented Dec 6, 2017

I wonder if we even leave off Associative here, as to allow for Vector{Pair}; then we could add tests w/ Vector{Pair} 😉

@samoconnor
Copy link
Contributor

I have a WIP experimental branch where I've replaced the internal representation of the headers with a Vector{Pair}. This seems to work well and makes dealing with special cases rules (like Set-Cookie) easier.
samoconnor@bb120bd

@samoconnor
Copy link
Contributor

@omus, mkheaders is now: mkheaders(h)::Headers = Header[string(k) => string(v) for (k,v) in h]
Does this cover the intention of your PR?

mkheaders(h)::Headers = Header[string(k) => string(v) for (k,v) in h]

@omus
Copy link
Contributor Author

omus commented Jan 18, 2018

Looks like this is still an issue:

HTTP.jl/src/client.jl

Lines 39 to 45 in f9837eb

function request(client::Client, method, url::URI;
headers::Dict=Dict(),
body="",
enablechunked::Bool=true,
stream::Bool=false,
verbose::Bool=false,
args...)

@samoconnor
Copy link
Contributor

Sorry for the confusion, the request(client::Client, ... method is there for compatibility.
See commit b5844b1

The new doc says:

HTTP.jl/src/HTTP.jl

Lines 41 to 43 in f9837eb

`headers` can be any collection where
`[string(k) => string(v) for (k,v) in headers]` yields `Vector{Pair}`.
e.g. a `Dict()`, a `Vector{Tuple}`, a `Vector{Pair}` or an iterator.

The top-level request method is now this:

HTTP.jl/src/HTTP.jl

Lines 293 to 301 in f9837eb

function request(method, url, h=Header[], b=nobody;
headers=h, body=b, query=nothing, kw...)::Response
uri = URI(url)
if query != nothing
uri = merge(uri, query=query)
end
return request(string(method), uri, mkheaders(headers), body; kw...)
end

mkheaders is now:

mkheaders(h)::Headers = Header[string(k) => string(v) for (k,v) in h]

@omus
Copy link
Contributor Author

omus commented Jan 22, 2018

Ah ok. If the headers keyword is now deprecated we should probably add a deprecation

@omus
Copy link
Contributor Author

omus commented Jan 22, 2018

FYI I managed to make a small test to demonstrate the issue (It appears that not all PyDicts cause the issue)

julia> using HTTP, PyCall

julia> @pyimport requests

julia> r = requests.get("http://httpbin.org/xml")
PyObject <Response [200]>

julia> h = PyDict(r[:request][:headers])
PyCall.PyDict{PyCall.PyAny,PyCall.PyAny,false} with 4 entries:
  "Connection"      => "keep-alive"
  "Accept-Encoding" => "gzip, deflate"
  "Accept"          => "*/*"
  "User-Agent"      => "python-requests/2.18.4"

julia> HTTP.get("http://httpbin.org/xml", headers=h)
ERROR: TypeError: #request: in typeassert, expected Dict, got PyCall.PyDict{PyCall.PyAny,PyCall.PyAny,false}
Stacktrace:
 [1] (::HTTP.#kw##request)(::Array{Any,1}, ::HTTP.#request, ::HTTP.Client, ::String, ::HTTP.URIs.URI) at ./<missing>:0
 [2] #get#23(::Array{Any,1}, ::Function, ::String) at /Users/omus/.julia/v0.6/HTTP/src/client.jl:144
 [3] (::HTTP.#kw##get)(::Array{Any,1}, ::HTTP.#get, ::String) at ./<missing>:0

When I use the positional parameter the request works as expected:

julia> HTTP.get("http://httpbin.org/xml", h)
HTTP.Messages.Response:
"""
HTTP/1.1 200 OK
...
"""

@samoconnor
Copy link
Contributor

Ah ok. If the headers keyword is now deprecated we should probably add a deprecation

No, its not deprecated:

HTTP.jl/src/HTTP.jl

Lines 293 to 294 in f9837eb

function request(method, url, h=Header[], b=nobody;
headers=h, body=b, query=nothing, kw...)::Response

Are you using latest master? This commit ought to fix that problem: b5844b1

@omus
Copy link
Contributor Author

omus commented Jan 22, 2018

The keyword works on the latest master.

@omus omus closed this Jan 22, 2018
@omus omus deleted the cv/associative-headers branch January 22, 2018 20:51
@omus
Copy link
Contributor Author

omus commented Jan 22, 2018

Why do we support headers as a positional and keyword parameter? If this has been discussed elsewhere could you point me to the discussion?

@samoconnor
Copy link
Contributor

#141 (comment)

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