On Thursday 07 July 2016 16:22:05 Richard W.M. Jones wrote:
Change the Curl module to use an ADT to store the name of the curl
binary and the arguments.
Good idea.
The implementation looks mostly good, although I have a couple of notes.
Also add Curl.safe_args, a list of arguments that control redirects etc.
The current implementation basically "forces" every caller to pass them
to Curl.create (and indeed this patch and patch #3 do this job), which
IMHO makes it easy to forget them.
Since we want those arguments on by default in all the curl invocations,
I'd do the following: hide Curl.safe_args, and add them automatically
in Curl.to_string. If in the future there will be the need to not use
them for some curl invocation, we can always add ~use_safe_args:false
for Curl.create.
+type proxy =
+ | UnsetProxy (** The proxy is forced off. *)
+ | ForcedProxy of string (** The proxy is forced to the specified URL. *)
+
+val args_of_proxy : proxy -> args
+(** Convert the proxy setting to the equivalent list of curl arguments.
+
+ To use the system proxy, no additional arguments are required, so
+ if you don't want to control the proxy (but just use the defaults)
+ you do not need to call this function at all.
+
+ Callers should append these arguments to the list of arguments
+ passed to {!create}. *)
Just wondering whether it makes sense a method to set the proxy for a
Curl.t handle, letting Curl do the proxy -> args conversion internally
(less work for users).
Thanks,
--
Pino Toscano