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
