On 24/08/11 19.33, Andrei Alexandrescu wrote:
On 8/24/11 9:35 AM, Walter Bright wrote:
On 8/24/2011 2:18 AM, Johannes Pfau wrote:
Walter Bright wrote:
On 8/17/2011 4:12 PM, David Nadlinger wrote:
the etc.curl module by Jonas Drewsen is at the front of the review
queue.
Thanks for doing this. I preface my remarks with saying I have never
done http: programming and know little about it.
1. The first, most obvious thing I'd like to do is present a URL and
get a string back that is the contents of that web page:
string contents = getURL("http://www.digitalmars.com";);
That's the first example in the API docs:
// Simple GET with connect timeout of 10 seconds
Http.get("http://www.google.com";).connectTimeout(dur!"seconds"(10)).toString();
I find this flow counterintuitive. It suggests that first a GET is
issued and then the timeout is set. It takes a bit of getting used to
things to realize that get() is actually preparing things and that the
real work is done in toString.
What I was aiming at was something like:
Http.get("http://google.com";, connectTimeout = dur!"seconds"(10));
But D does not support named parameters and that is why it ended up this
way.
I guess renaming to Http.prepareGet("http://google.com";) or something
would better reflect its usage.
Ok, but 1. there's no indication in the doc that that's what it does
because 1. it is an incomplete fragment and 2. a simpler interface (like
what I proposed) would likely be better.
or, if you don't care about timeouts:
string contents = Http.get("http://www.google.com";).toString();
This example needs to be first and needs an explanation. Timeouts, error
recovery, progress indicators, etc., should all be brought up later.
I'll note there's an annoying redundancy: the protocol appears twice,
once in the URL and again in the type Http. That means if one needs to
fetch from an URL transparently they'd have to go through e.g.:
string url = readln();
string contents;
if (url.startsWith("http://";)) {
contents = Http.get(url).toString();
} else if (url.startsWith("ftp://";)) {
contents = Ftp.get(url).toString();
} else {
enforce(false, "Unrecognized protocol.");
}
Probably this simple level of abstraction should be provided by the
library (with the added advantage that client code will automatically
work with new protocols as the library includes them).
Someone also got confused about the "get()" static method could be
called on an instance of Http. This is of course not how it is intended
to be used but never the less possible. I guess forcing the user to
prepend the class name when calling a static class method would be nice
to catch such errors. Maybe this should be change in the D language?
Since this is currently not the case I think moving all the
Http.get(),Http.post()...Ftp.get()... static methods to module scope
would solve the issue.
I also think it would be a good idea to provide a function that parses
the protocol and just performs the curl request as a convenience. This
cannot replace the Http.get() style functions because they return a
Result that know what can be done on a specific protocol. For example
maxRedirects is supported for the Http protocol but not for Ftp/Smtp.
So to summarize:
* Move static convenience methods (Http.get/post/...) to module level
functions and rename to reflect that they only prepare a request. One of
toString(),bytes(),byLine(),byChunk(),byLineAsync(),byChunkAsync() will
have to be called to actually get something.
* Create a new convenience function that parses protocol from url and
performs a sync version of the request and then returns the result
(ubyte[] or string). This is not as flexible but probably the most
common case. For example:
string content = curl("http://mysite.com/form.cgi";,
"hello world data",
"username:password");
Actually would adding opCast!string() and opCast!(ubyte[])() to
AsyncResult and Result allow for this?:
string content = Http.get("http://mysite.com/form.cgi";);
or
ubyte[] content = Http.get("http://mysite.com/form.cgi";);
How does this sound?
I'll be back with a thorough review.
Andrei
