CURL review request

[Jonathan M Davis]

On Tuesday, August 16, 2011 20:23:19 jdrewsen wrote:
> Den 16-08-2011 15:13, dsimcha skrev:
> > On 8/16/2011 7:48 AM, Jonas Drewsen wrote:
> >> Hi all,
> >> 
> >> This is a review request for the curl wrapper. Please read the "known
> >> issues" in the top of the source file and if possible suggest a
> >> solution.
> >> 
> >> We also need somebody for running the review process. Anyone?
> >> 
> >> Code:
> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
> >> Docs:
> >> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
> >> 
> >> Demolish!
> >> 
> >> /Jonas
> >> 
> >  From a quick look, this looks very well thought out. I'll review it
> > 
> > more thoroughly when I have more time. A few questions/comments from a
> > quick look at the docs:
> > 
> > Does the async stuff use D threads, or does Curl have its own async API?
> 
> It uses the spawn method from std.concurrency which uses D threads i think.
> 
> > In your examples for postData, you have onReceive a ubyte[] and write it
> > out to console. Did you mean to cast this to some kind of string?
> 
> Not really. It is just an example and writeln will output a byte array
> just fine.
> 
> > For onReceive, what's the purpose of the return value?
> 
> To specify the number of bytes that have been handled. If this is less
> that the entire buffer the rest of the request will fail. Additionally
> both the onSend and onReceive callbacks can return  CURL_WRITEFUNC_PAUSE
> pr CURL_READFUNC_PAUSE to pause the transfer. And onSend can return
> CURL_READFUNC_ABORT to abort as well. I see that the is missing from the
> docs... will add.
> 
> > If/when this module makes it into Phobos, are we going to start
> > including a libcurl binary with DMD distributions so that std.curl feels
> > truly **standard** and requires zero extra configuration?
> 
> I think that would be convenient but don't believe it is the correct
> thing to do. In my opinion D modules that are not pure D or wrapping a
> system library (which libcurl is definitely not) should be the only ones
> allowed in the std package. Other wrappers in phobos should be in the
> etc package and the lib binaries should not be included.
> 
> Just how I feel about it though. Anyone who disagrees?

In general, external libraries should not be included in Phobos. They should 
be using whatever is on the user's system. As such, you're correct in that etc 
is probably the place to put the curl wrapper, not std.

- Jonathan M Davis