On Saturday, 17 December 2011 at 22:25:07 UTC, Andrei
Alexandrescu wrote:
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
ok
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
ok
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."
ok
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.
Use byLine/Chunk with a connection initialized with post/put data:
auto http = HTTP();
http.postData = "hello world";
foreach (line; byLine("dlang.org/there", KeepTerminator.no, '\n',
http))
writeln(line);
Same deal for directing to file using download()
5. Also there seems to be no way to issue a PUT/POST from a
range.
Correct. It will be a later extension.
6. s/Examples:/Example:/
ok
7. s/HTTP http = HTTP("www.d-p-l.org");/auto http =
HTTP("www.d-p-l.org");/
ok
8. "receives a header or a data buffer" -> "receives a header
and a data buffer, respectively"
ok
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.
ok
10. "Finally the HTTP request is started by calling perform()."
-> "Finally the HTTP request is effected by calling perform(),
which is synchronous."
ok
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.
ok
12. The example for InferredProtocol nee AutoConnection does
not show any interesting example, e.g. ftp.
improved
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.
Using the same method as for post it is possible to set timeouts.
auto http = HTTP();
http.dataTimeout = dur!"minutes"(2);
foreach (line; byLine("dlang.org/there", KeepTerminator.no, '\n',
http))
writeln(line);
Regarding uploading ranges this will be handled by the same
extension as
for POST/PUT output ranges.
14. Parameter doc for put(), post(), del() etc. is messed up.
ok
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...
But you can provide an existing connection to use has that is
already connected to the server. Hence the name connection.
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.
ok - improved
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.
This will change when multi API is supported as well.
18. In byXxxAsync wait(Duration) should be cross-referenced to
its definition.
ok
=========================
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.
They can abort from the callback handlers - but otherwise it is a
limitation of the libcurl easy API.
To actually do this the multi extension would have to be added.
Thank you for the comments and sorry for the late reply.
/Jonas
