Second Round CURL Wrapper Review

[Andrei Alexandrescu]

On 12/2/11 10:26 PM, 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?

Yes.

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

std.net.curl

> Code: https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
> Docs: http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

I have a few comments. They are not critical for library's admission 
(apologies I didn't make the deadline) and could be integrated before 
the commit.

1. Tables must be centered (this is a note to self)

2. Replace d-p-l.org in examples with dlang.org

3. "Compared to using libcurl directly this module provides a simpler 
API for performing common tasks." -> "Compared to using libcurl directly 
this module allows simpler client code for common uses, requires no 
unsafe operations, and integrates better with the rest of the language."

4. From the high level ops it seems there's no way to issue a PUT/POST 
and then direct the result to a file, or read the result with a foreach.

5. Also there seems to be no way to issue a PUT/POST from a range.

6. s/Examples:/Example:/

7. s/HTTP http = HTTP("www.d-p-l.org");/auto http = HTTP("www.d-p-l.org");/

8. "receives a header or a data buffer" -> "receives a header and a data 
buffer, respectively"

9. "In this simple example, the headers are writting to stdout and the 
data is ignored." How can we make it to stop the request? A sentence 
about that would be great.

10. "Finally the HTTP request is started by calling perform()." -> 
"Finally the HTTP request is effected by calling perform(), which is 
synchronous."

11. Use InferredProtocol instead of AutoConnect(ion). The type is 
exactly what it claims. If "inferred" is too long, "AutoProtocol" would 
be fine too. But it's not the connection that is automated. The word 
must not be a verb, i.e. InferProtocol/DeduceProtocol would not be good.

12. The example for InferredProtocol nee AutoConnection does not show 
any interesting example, e.g. ftp.

13. I think download() and upload() are good convenience functions but 
are very limited. Such operations can last arbitrarily long and it's 
impossible to stop them. Therefore, they will foster poor program design 
(e.g. program hangs until you kill it etc). We should keep these for 
convenience in scripts, and we already have byXxx() for downloading, but 
we don't have anything for uploading. We should offer output ranges for 
uploading sync/async. Maybe in the next minor release.

14. Parameter doc for put(), post(), del() etc. is messed up.

15. I don't think "connection" is the right term. For example we have

T[] get(Conn = AutoConnection, T = char)(const(char)[] url, Conn conn = 
Conn());

The connection is established only during the call to get(), so conn is 
not really a connection - more like a sort of a bundle of connection 
parameters. But then it does clean up the connection once established, 
so I'm unsure...

16. For someone who'd not versed in HTTP options, the example

string content = options("d-programming-language.appspot.com/testUrl2", 
"something");

isn't terribly informative.

17. In byLine: "A range of lines is returned when the request is 
complete." This suggests the last byte has been read as the client gets 
to the first, which is not the case. "Data will be read on demand as the 
foreach loop makes progress." Similar for byChunk.

18. In byXxxAsync wait(Duration) should be cross-referenced to its 
definition.

=========================

Overall: I think this is a valuable addition to Phobos, but I have the 
feeling we don't have a good scenario for interrupting connections. For 
example, if someone wants to offer a "Cancel" button, the current API 
does not give them a robust option to do so: all functions that transfer 
data are potentially blocking, and there's no thread-shared way to 
interrupt a connection in a thread from another thread.

Such functionality may be a pure addition later, so I think we can 
commit this as is. Please use std.net.curl.


Thanks,

Andrei