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