CURL review request

[jdrewsen]

Den 16-08-2011 15:54, Alix Pexton skrev:
> 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..."

ok

> 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 >< )

The first collected example? Can you tell me the comment above the 
example you do not understand?

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

This is how it is for all modules AFAIK.

> 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."

ok

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

ok

> Protocol.onProgress represents transfer statistics using doubles?

That is what we get from libcurl

> 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?

I will include a link to the HTTP RFC that explains it.

> 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?

Added explanation and a link to HTTP RFC explaining sematics.

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

ok

> 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.

ok

> Also, later examples are just marked as "Asynchronous version
> of..." and lose the note about callbacks, which I assume still apply.)

The latter examples are trivial HTTP methods not sending or receiving 
anything but the status codes are used. Therefore they do not need an 
elaborate example as for the GET/POST/PUT methods.

> 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.

It is possible to provide read functionallity of the properties. The 
only issue with this is that the delegate read would not be the same as 
the one just written because it gets wrapped it the propery set call. I 
don't know if this is a problem though?

> 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 ^^

That's a bug. Will fix.

> 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."

ok

> 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.

That ddoc standard config needs a patch to support etc module xrefs.

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

I will try to improve the docs. They return the EncodingSchemes that is 
used when reading the data

http://www.d-programming-language.org/phobos/std_encoding.html

The scheme is changed by using the encodingName property.

> ---
>
> 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.

Maybe you're right. Whether it is a product of being a wrapper around an 
existing library and thereby forcing some design decisions or by my 
coding style I don't know.

If anyone can give some input as how to make it more D'ish please do.

Thank you the valuable feedback
/Jonas

> A...
>