Second Round CURL Wrapper Review

[Jonathan M Davis]

On Friday, December 02, 2011 23:26:10 dsimcha wrote:
> I volunteered ages ago to manage the review for the second round of
> Jonas Drewsen's CURL wrapper.  After the first round it was decided
> that, after a large number of minor issues were fixed, a second round
> would be necessary.
> 
> Significant open issues:
> 
> 1.  Should libcurl be bundled with DMD on Windows?

I'd argue that it should be a separate download but that it should definitely 
be provided.

> 2.  etc.curl, std.curl, or std.net.curl?  (We had a vote a while back
> but it was buried deep in a thread and a lot of people may have missed
> it:  http://www.easypolls.net/poll.html?p=4ebd3219011eb0e4518d35ab )

If we're going to follow a policy that wrappers around 3rd party libraries go 
in etc, then it should be etc.curl. Otherwise, I think that it should be 
std.net.curl.

I'd argue that AutoConnection should be AutoConnect, because it's shorter and 
just as informative.

I'd argue that the acronyms should be in all caps if camelcasing would require 
that the first letter of the acronym be capitalized and all lower case if 
camelcasing would require that the first letter of the acronym be lower case. 
We're not completely consistent in how we using acronyms in names in Phobos, 
but I believe that we primarily follow that rule when putting them in symbol 
names. So, for instance, it would be HTTP, FTP, and SMTP rather than Http, 
Ftp, and Smtp.

I'd rename T to C or Char in the template parameters, since it's a character 
type rather than a generic type. Not a huge deal, but I think that it makes 
the code quicker to understand.

Why are some parameters const(char)[] and others string? Why not make them all 
const(char)[] or even actually templatize them on character type (one per 
parameter which is a string)?

Shorten the URL in your examples. Its long length increases the risk of making 
the examples too wide. It's a made up URL, so there's no need for it to be 
that long. Something like foo.bar.com is probably more than enough.

Why does byLine use '\x0a' instead of '\n'? '\n' is clear, whereas most people 
will have to look up what '\x0a' is, so I really think that it should be '\n'. 
Also, if byLine is going to return an internal range type instead of having an 
externally defined one, its documentation needs to explain what Char is used 
for. As it stands, it looks like a pointless template parameter. To do that, 
the return section should probably say something like "A range of Char[]..." 
All of this goes for byLineAsync as well.

In general, the functions should do a better job of documenting their 
parameters. Many of them don't document their parameters at all. For instance, 
while it's fairly easy to guess what keepTerminator and terminator are used 
for, there is no explanation about them whatsoever in either byLine or 
byLineAsync's documentation.

If any of the range return types of these functions have non-standard 
functions on them, they need to be properly documented. e.g. wait(Duration) is 
mentioned in passing in byLineAsync's documentation, but no explanation for 
its purpose is given. The documentation needs to explain everything the 
programmer needs to know about the return types. For most ranges, it's enough 
to say that the return type is a range of X, but if the return type has 
anything other than the standard range functions, then just saying that the 
function returns a range of X is not enough. The extra functions must be 
listed and explained.

I'd warn you that the fact that the documentation for Protocol is showing up 
in the documentation is likely a bug, since Protocol is private - probably the 
bug that all templates are currently public even if they're marked private. 
So, either Protocol needs to be public (or maybe protected), or it's going to 
disappear from the documentation eventually. Another option would be to do 
something similar to std.container.TotalContainer and declare a struct which 
is intended only for documentation (probably in a version(StdDdoc) block), and 
put all of the protocol documenation on _it_ instead of the mixin.

Also, Protocol needs a better explanation as to what it's actually for. "Mixin 
template for all supported curl protocols" doesn't really tell me anything 
other than the fact that it relates to the supported protocols somehow. 
Looking at the source, it seems clear enough, but not from the documentation - 
especially when the documentation seems to imply that it's specific to the Http 
struct.

onReceiveHeader has const(char)[] parameters but mentions char[] in its 
documentation. The meaning is clear enough, but it would be more correct to 
say const(char)[] or to just mention the variable names explicitly.

It would be nice if Smtp.mailTo was variadic  - i.e. mailTo(string[] 
recipients...). All of the current use cases for it would be exactly the same 
with the addition of being able to do something like mailTo(str1, str2, str3).

The exception type's constructors should look like this:

    nothrow this(string msg, string file = __FILE__, size_t line = __LINE__, 
Throwable next = null)
    {
        super(msg, file, line, next);
    }

As it stands, the line number will be completely wrong. You can just copy 
std.string.StringException and rename it. And please use that exact signature. 
Don't make it take a const(char)[]. Exception requires a string, and with the 
constructor taking const(char)[], it's going to needlessly idup every string 
that gets passed into it. The code which calls it can idup if it has a char[] 
instead of an immutable(int)[]. Having it take a const(char)[] adds no value 
and is less efficient. The same goes for any other function which is always 
going to idup its const(char)[] argument. Just make it take a string.

Why do CURLoption and CURLcode  have CURL in uppercase? That doesn't match the 
rest of the module. I'd argue that they should be CurlOption and CurlCode. To 
make matters even weirder, your examples appear to use CurlOption but the 
declarations use CURLoption. They should be consistent.

- Jonathan M Davis