Second Round CURL Wrapper Review

[Jonathan M Davis]

Line# 235 is identical to line# 239. Shouldn't line# 235 be creating an Http 
object, not an Ftp object? That mistake definitely makes it look like download 
hasn't been properly tested.

You should create template similar to

template isCurlConn(Conn)
{
    auto isCurlConn = is(Conn : Http) || is(Conn : Ftp) || is(Conn : 
AutoConnection);
}

It would reduce code duplication.

The functions which have a template parameter T = char really need to have 
that parameter properly explained in the documentation. From the 
documentation, I would have thought that it was any character type, but it's 
char or ubyte. There is no hint whatsoever in the documentation that ubyte 
would work. All of the parameters need to be made clear. That goes for all 
functions.

Is there a reason that the functions which have a template argument T which 
can be either a char or a ubyte can't work with immutable char? Glancing at 
_basicHttp, I don't see any reason why T couldn't be immutable char. Yes, it 
would require casting to immutable(char)[], but you're already casting to 
char[], and the data being returned appears to be unique such that it could be 
safely cast to immutable. That being the case, I'd encourage you to not only 
make it work with immutable char but to make immutable char the default 
instead of char.

Once you've fixed the exception types like I requested in my first review, you 
can and should use enforceEx!CurlException(cond, msg) instead of enforce(cond, 
new CurlException(msg)).

it may not matter, since we're dealing with strings here, but I'd argue that 
!empty should be used rather than length > 0 (e.g. line# 475). In stuff other 
than strings it definitely can be more efficient, and even with strings, it may 
be, since I think that checking whether the length is 0 (as empty does) is 
slightly more efficient than checking whether it's greater than 0.

Please make sure that opening braces on on their own line. That's the way that 
the rest of Phobos does it and it's one of the few formatting rules that we've 
been trying to use consistently throughout Phobos. For the most part, you get 
it right, but not everywhere - nested functions in particular seem to have 
braces on the same line for some reason.

It's not a big deal, but you should use auto more. For instance, lines #626, 
#638, and #640 don't use auto when they could.

I have no idea why you keep putting parens around uses of the in operator, but 
it's not necessary and makes the code harder to read IMHO. It's certainly not 
required that you change that, but I'd appreciate it if you did.

I see that regexes are used in the module. Please make sure that they still 
work correctly with the new std.regex. They probably do, but it's not 100% 
backwards compatible.

byLine's template constraint needs to verify that the types of Terminator and 
Char are valid. As it is, I could try and pass it something like an Http 
struct if I wanted to. In general, _all_ templates need to verify that their 
arguments are of the appropriate type for _all_ of their arguments.

It's odd that popFront (line #780) is not @safe when empty and front are. Does 
findSplit or findSplitAfter prevent it?

With a name like Char on byLine, I'd expect it to take any char type, but not 
only do you not verify the type (as previously mentioned), but you instantiate 
_basicHttp with it, which works with char and ubyte, not any char. If Char is 
going to take char and ubyte specifically, then Char is a bad name for it.

I'd suggest changing line# 823 to

auto result = get(url, isFtpUrl(url) ? FTP() : Http());

It would save several lines of code.

Line# 883 should be using +=.

This would be easier if I could comment on the code directly, but I don't seem 
to be able to with the link that dsimcha provided (probably because it's blob; 
there's probably another link which would allow direct commenting, but oh 
well). I'll comment more on the implementation later, but bed beckons...

- Jonathan M Davis