Second Round CURL Wrapper Review

[jdrewsen]

On Thursday, 15 December 2011 at 08:51:29 UTC, Jonathan M Davis 
wrote:
> 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.

It has been tested as you can see by the unittest just below the 
function. It just did not fail because libcurl requires the same 
setup for this download situation for both ftp and http. Will fix 
it of course.

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

ok

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

ok

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

Is is indeed unique and can be cast to immutable. I'll add that 
option.
Why do you think immutable char as the default is better than 
char? I know that the return type in that case would be string 
and not char[] - but why is that better?

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

ok

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

ok

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

When I look through the code it seems ok with regards to braces 
on own line. Maybe by nested functions you mean delegates? The 
delegates I use do indeed have braces on same line which is ok 
afaik.

> 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'm a bit in doubt. On one hand it is great to make everything 
auto in order to make it easy to change type and to remove 
redundancy. One the other hand it is very convenient to be able 
to see the type when you read the code. I know that a clever 
editor would be able to figure out the type for me. But I also 
read code in normal editors for diffs etc. or on github.

Anyway - I've changed as much as possible to auto now.

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

Bad habit. Will remove.

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

ok

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

ok

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

exactly

> 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'll verify the type. For byLine it will only accept char types.

> I'd suggest changing line# 823 to
>
> auto result = get(url, isFtpUrl(url) ? FTP() : Http());

Not possible. get() is a template function where the template 
parameter is connection type ie. one of FTP or HTTP. 
isFtpUrl(url) is evaluated at runtime and not compile time.

> It would save several lines of code.
>
> Line# 883 should be using +=.

ok

> 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

Thanks for this second round of comments

/Jonas