CURL review request

[Alix Pexton]

On 16/08/2011 12:48, 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


Here are my initial observations and impressions...

In the opening description...
"The request is perform when first accessing..." -> "The request is 
performed when first accessing..."

On first reading, I found it very hard to follow the first collected 
example. (Still not 100% clear after reading the whole of the 
documentation either >< )

Some (all?) of the @property members of Protocol are documented as 
functions. (Perhaps an artefact of the ddoc mixin issue mentioned before 
this section?)

Description of Protocol.onSend...
"...specifies the max number of bytes that can be send." -> 
"...specifies the maximum number of bytes that can be sent."
"...returns the number of elements in the buffer that has been filled 
and is ready to send." -> "...returns the number of elements in the 
buffer that have been filled and are ready to send."

Description of Protocol.onReceive...
"...is not guaranteed to be valid aften the callback returns." -> "...is 
not guaranteed to be valid after the callback returns."

Protocol.onProgress represents transfer statistics using doubles?

CurlTimeCond, members are all lower case, took me a few goes to work out 
what they might mean, still not all that sure. Should I assume that if 
the meanings are not obvious that I'm not ready to be tinkering with 
curl at this level, or should each value be clarified?

Http.addHeader / Http.setHeader both have the same description, I assume 
the latter overwrites an existing value if it exists? what is the 
behaviour of addHeader when the key already exists?

Http.setTimeCondition parameter names in ddoc don't match those in the 
function prototype.

Http._whatever_Async (and also Async functions for Ftp.)
"Asynchronous HTTP _WHATEVER_ to the specified URL." -> "Performs a HTTP 
_WHATEVER_ on the specified URL, asynchronously."
(Wording looks very similar across this group of functions, not sure if 
my suggested alteration here correctly captures the subtleties for all 
cases. Also, later examples are just marked as "Asynchronous version 
of..." and lose the note about callbacks, which I assume still apply.)

All callbacks are write only properties which prevents the use of event 
hooking (which, in my experience, is a common occurrence in event based 
models). Hooking might not apply in this case however (I've not 
particularly thought through any use cases) and safe event hooking might 
be beyond the scope of this module.

Http.traceAsync
Calls AsyncResult with Method.get? I would have expected Method.trace, 
but I only spotted this by chance, could very well be my ignorance 
showing ^^

Http.onReceiveStatusLine
"...several callbacks can be done for on call to perform() because if 
redirections." -> "...several callbacks can be done for each call to 
perform() [because of | due to] redirections."

Http.authenticationMethod
std.etc.c.curl.AuthMethod? I think this is an issue with the XREF macro 
not expecting the link to break out of the std package.

Ftp.Result.encoding / Ftp.AsyncResult.encoding
I'm not sure what these functions are supposed to return, the 
descriptions seem inadequate.

---

Not fully read all the source yet, but as I'm not all that familiar with 
curl, I'm not sure I'll spot much ^^

Not a criticism, but at first glance, it doesn't feel very D-ey, but I'm 
still not 100% sure what idiomatic D is yet.

A...