CURL review request

[jdrewsen]

Den 16-08-2011 21:59, Jonathan M Davis skrev:
> 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

It could ofcourse just be make a root module 'curl' instead of 
'etc.curl'. I would like it to be etc.curl in phobos since I think D 
needs a set of "Phobos approved" extensions. Extensions that are 
guaranteed to have certain amount of quality because they have been 
through the review process.

Maybe at some point when a package manager of some kind is available the 
modules in the etc package can be torn out of the official phobos bundle 
and downloaded separately from d-p-l.org.

/Jonas