Open hkalexling opened 4 years ago
Looks good for me! :+1:
However, the get_proxy
method could return nil, resulting in the following error
In src/utils/proxy.cr:33:26
33 | client.set_proxy get_proxy uri
^--------
Error: expected argument #1 to 'HTTP::Client#set_proxy' to be HTTP::Proxy::Client, not (HTTP::Proxy::Client | Nil)
This error can of course be easily avoided
if prxy = get_proxy(uri)
client.set_proxy prxy
end
Thanks!
Hi @hkalexling @kojix2 I apologize for the delay in addressing this issue. Could you please create a pull request for this? I would greatly appreciate it. Thank you.
Long time no see.
For some reason, the above code didn't work on Crest. I haven't looked into why. I wrote a workaround like this:
module Crest
class Request
def initialize(
method : Symbol,
url : String,
form = {} of String => String,
**args
)
previous_def
url = URI.parse(@url)
no_proxy = ENV["no_proxy"]? || ENV["NO_PROXY"]?
return if no_proxy && no_proxy.split(",").includes?(url.host)
key = "#{url.scheme}_proxy"
proxy_ = ENV[key.downcase]? || ENV[key.upcase]?
return if proxy_.nil?
uri = URI.parse proxy_
@p_addr = uri.host
@p_port = uri.port
@p_user = uri.user
@p_pass = uri.password
set_proxy!(@p_addr, @p_port, @p_user, @p_pass)
end
end
end
However, this workaround breaks as soon as the signature of the Crest::Request#initialize
method changes.
It would be better to handle it on the https_proxy side.
@kojix2 Which specific piece of code are you referring to?
The
[protocol]_PROXY
andNO_PROXY
environment variables are commonly used to configure proxies in applications. Wget and Emacs are two examples. Python'srequests
library also respectsHTTP_PROXY
andHTTPS_PROXY
out of the box.It would be great if we could have a helper method like
HTTP::Proxy::Client.use_env
that would do the followingsHTTP::Proxy::Client
objects.HTTP::Client
class in the standard library so that the clients use the appropriate proxies in all requests.Here's an example implementation I used in my project. I will be happy to submit a PR if you think it's a good idea.