Second Round CURL Wrapper Review

[jdrewsen]

On Thursday, 15 December 2011 at 07:46:56 UTC, Jonathan M Davis 
wrote:
> 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.

AutoConnect sounds like a command to connection automatically 
which might be confusing since that is not what it does. 
Therefore I went with AutoConnection which I still believe is 
better.

> 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 like Http better as personal taste. But if HTTP is preferred 
then I'll do that. Actually I think I will make a pull request to 
extend the dlang.org/dstyle.html doc with and example that we can 
point to when someone asks about styling. I've spend too much 
time restyling because there is no such style doc already and 
people are complaining about style in reviews anyway.

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

I've named it T because it can also be ubyte. In that case C for 
Char is confusing.

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

The parameters that are const(char)[] will accept both string and 
char[] which is what I want because libcurl make an internal copy 
of it anyway. If I typed it as string then you would have to idup 
a char[] for the parameter without any reason.
The parameters that are string is internally passed to functions 
(in other std modules) that only accept strings. I could accept 
const(char)[] and idup it in order to have consistant parameter 
types but that gives an unnecessary overhead.

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

Actually the long urls point to a google appspot app that fits 
the example. I've done it this way so that people can just 
copy/paste the example and have something working. Not too many 
people have a test HTTP POST server setup for a quick test AFAIK.

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

I looked at how it was done elsewhere in phobos and did like that:
http://dlang.org/phobos/std_stdio.html#byLine

But I agree with you that \n is better and will change it.

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

Ok.

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

Again I looked at phobos to see what was current the style:
http://dlang.org/phobos/std_stdio.html#byLine

I'll include some more detail though.

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

This is what is currently documented: "If no data is available 
and the main thread access the range it will block
until data becomes available. An exception to this is the $(D 
wait(Duration))
method on the range. This method will wait at maximum for the 
specified
duration and return true if data is available."

I guess that explains it?

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

Ok. didn't know about the private template bug. I really think it 
is a bug in ddoc that it should be able to insert mixed in docs 
ie. the Protocol documentation should be mixed into the Http 
documentation.

Anyway - I don't see the TotalContainer docs showing in the 
generated html?

Another workaround is to manually copy the Protocol docs into the 
HTTP/FTP/SMTP structs inside a version(StdDdoc).

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

I'll remove the HTTP hint and improve the doc.

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

ok.

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

ok

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

ok

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

ok

Thanks for the comments.

-Jonas