Second Round CURL Wrapper Review

[jdrewsen]

On Monday, 12 December 2011 at 00:53:14 UTC, dsimcha wrote:
> Here's my review.  Remember, review ends on December 16.
>
> Overall, this library has massively improved due to the rounds 
> of review it's been put through.  I only found a few minor 
> nitpicks.
> However, a recurring pattern is minor grammar mistakes in the 
> documentation.  Please proofread all documentation again.
>
> Docs:
>
> "The high level API is build" -> "The high level API is built"
>
> "LibCurl is licensed under a MIT/X derivate license" -> 
> "LibCurl is licensed under an MIT/X derivative license"
>
> AutoConnect:  "Connection type used when the url should be used 
> to auto detect protocol."  ->  "auto detect THE protocol"

ok

> Why is there a link to curl_easy_set_opt in the byLineAsync and 
> byChunkAsync docs?

will fix

> In onSend:  "The length of the void[] specifies the maximum 
> number of bytes that can be send." -> "can be SENT"

ok

> What is the use case for exposing struct Curl?  I prefer if 
> this were unexposed because we'll obviously be unable to 
> provide a replacement if/when the backend to this library is 
> rewritten in pure D.

Initially it was not exposed but there were a couple of requests 
to expose it. Thats why.

> Actually, that leads to another question:  Should this module 
> really be named etc.curl/std.curl/std.net.curl, or should the 
> fact that it currently uses Curl as a backend be relegated to 
> an implementation detail?

I think it should have curl in the name. I do not believe that a 
native D network library should have the same API as this module. 
It is limited by the design of libcurl and should be improved by 
a native D net library.

> Code:
>
> pragma(lib) basically doesn't work on Linux because the object 
> format doesn't support it.  Don't rely on it.

ok

> Should the protocol detection be case-insensitive, i.e. 
> "ftp://" == "FTP://"?

yes


Thanks for the feedback
/Jonas