CURL review request

Alix Pexton alix.DOT.pexton at gmail.DOT.com
Tue Aug 16 15:07:22 PDT 2011


On 16/08/2011 20:44, jdrewsen wrote:
> Den 16-08-2011 15:54, Alix Pexton skrev:
>
>> Protocol.onProgress represents transfer statistics using doubles?
>
> That is what we get from libcurl

I'll let you off then, but just this once ^^

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

Seems reasonable.

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

I read the source now, they do do as I expected, which made me wonder 
why there is no removeHeader, but I can't think of a reason to use 
removeHeader, so I won't suggest its addition ^^

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

Well, my honest thought as I read through it was that you were getting 
fed up of writing near identical documentation for each method and had 
started to cut corners. Can ddoc be used to add a section in front of 
these functions that outlines their common features? Making it a bit DRYer?

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

Hmn, it could well be! I've not thought of a good use case yet though, 
so I'd recommend leaving it as it is for now, and if D gets an Events 
module in the future, the hooking issue can be revisited.

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

I looked at the code, and for a moment I though I got it, but I was wrong ><
from the source...
> ref Result encoding(const(char)[] schemeName) {
>             rp._encodingSchemeName = schemeName;
>             return this;
>         }

I'm now guessing this is just for chaining?

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

The source, for the most part, does feel D'ish, so you must be doing 
something right, but there is no point in adding extra templates and 
custom ranges if there is no need. I'll have a stab at writing some code 
with it tomorrow, to see how the bits fit together in the wild.

> Thank you the valuable feedback

No problem, I'm glad you found it so ^^

A...


More information about the Digitalmars-d mailing list