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
