On 8/17/11 6:12 PM, David Nadlinger wrote:
Now that Lars Kyllingstad's new and improved std.path has passed the
vote – congratulations, Lars! –, and Jose Armando Garcia, the author of
the proposed logging module, is currently not available, the etc.curl
module by Jonas Drewsen is at the front of the review queue. I have
volunteered to run the formal process for inclusion with Phobos.
[snip]
My review of etc.curl follows.
================
Overall
================
The library is comprehensive but does not cater to frequent use cases.
It could be metaphorically compared with a remote control with
equally-sized and spaced buttons, as opposed to one with enlarged
frequent use buttons.
There are two prominent use cases:
1. Given a URL string gimme the content (binary or string). The URL
could be even "file://".
2. Given a URL as in (1) gimme a method to go through its content one
chunk or one line at a time. The method should be efficient (i.e.
support asynchrony transparently such that client code and downloading
code don't block each other).
I see these cases together covering a vast majority of use cases; (1)
would be for quick and dirty scripts, (2) would be for applications that
do casual networking. The only apps that need advanced APIs would be
those for which networking is a central feature.
Neither of these use cases is covered adequately by the proposed API.
Furthermore, the documentation is also inadequate. Description is
scarce, cross-references are missing, and examples are poor and scarce.
A major overhaul is needed for acceptance in Phobos.
Finally, I found a couple of egregious bugs related to buffer overflow,
both in the examples and the implementation. There should be significant
scrutiny of such. Ideally we'd frame these issues as an API design
problem that we may be able to design around.
My vote is contingent upon fixing these large issues. Below are more
details along with a variety of smaller comments.
================
Documentation
================
* Should be std.curl.
* "Curl client functionality as provided by libcurl." -> "Networking
client functionality as provided by libcurl."
* Code inlined within text should use $(D ...).
* The first example is meaningless:
Http.get("http://www.google.com";).connectTimeout(dur!"seconds"(10)).toString();
It gets a string but throws it away. At best it's a test that google.com
is up (it would throw otherwise).
* "Http http = Http("http://www.google.com";);" -> "auto http =
Http("http://www.google.com";);" No stuttering please. Also Http's
constructor should imply "http://"; if not present, so "auto http =
Http("www.google.com");"
* "foreach (line; Http.getAsync("http://www.google.com";).byLine())" is
rather verbose considering it may be the most frequent use pattern. I
was hoping for e.g. "foreach (line; Http.byLine("google.com"))"
* This comment is inaccurate:
// Do some expensive processing on the line while more lines are
// received asynchronously in the background.
The idea of async in here is seldom to do expensive computation, but
rather to offer incremental processing and output (and sometimes to stop
early).
* In the "put with data senders" it's unclear what the length of data is
and what happens if that length is shorter than msg.length. Setting
contentLength to 11 is awkward. Surely there must be an easier API for
sending some string up the wire.
* "Smtp smtp = Smtp("smtps://smtp.gmail.com");" -> "auto ..."
* "This documentation should really be in the Http..." -> we need to
find a solution to this prior to approval. Perhaps version(StdDdoc) may
be of help.
* Documentation for individual symbol lacks examples. For example, one
has no idea what requestPause is about, and there's no cross-reference
sending to the function that uses or returns it. Each symbol or symbol
group should have examples.
* Documentation is extremely scarce and uses terms without defining
them. It's reasonable to assume what escape() and unescape() do in such
a module, but for example cleanup() says "Stop and invalidate this
instance." which gives no indication of the meaning of invalidation.
Also, isStopped() checks whether the object is "stopped and invalidated"
but effecting stopping and invalidation is not done by stop(), but by
cleanup().
* verbose, dataTimeout etc. don't have corresponding properties for reading.
* netInterface takes a string but not found ubytes (or an IP struct).
this means someone who has the IP in another format must format it as a
string first.
* localPort() should take a ushort, not an int.
* Phrases such as "tcp nodelay socket option" should link to relevant
off-site documentation or a glossary entry.
* onSend example is unindented, uses "l" as a symbol, and is liable to
buffer overflow, which is, to be brutally honest, appalling.
* The example for onReceive should use to!(const(char)[]) instead of the
cast, such that the UTF validity is checked.
* Since neither value in onProgress can be fractional, the type should
be size_t or ulong.
* Due to probably a bug in ddoc, the members of struct Http are not
indented.
* The informal examples given with addHeader and setHeader should also
be supported by code examples.
* Example for head() misses a semicolon.
* Documentation for post() makes it easy to overlook the important
distinction between the two overloads. The documentation should explain
how one is meant for text upload and the other for binary upload.
* optionsAsync has an extra paren.
* The various HTTP primitives wrapped (del, connect, ...) should include
links to their definition in the documentation.
* The dox for the two postData() overloads should be merged.
* Until the documentation for perform() comes about towards the end of
the dox, it's unclear that a lot of work is done by it. So someone who
wants to understand the library without having first learned curl would
need to infer this fact from the scarce examples.
* Again, many properties have setters but not getters (e.g. maxRedirects).
* The word "simply" is overused.
================
Implementation
================
* We usually import modules in alphabetical order.
* The frequent pattern if (!expr) throw new CurlException(msg); should
be replaced by enforce(expr, new CurlException(msg));
* throwOnStopped should accept a defaulted string. There are several
instances of the pattern if (stopped) throw new CurlException("...");
* I think we can in the future replace curl_easy_(un)escape with more
efficient native functions.
* This kind of stuff is unsafe: cast(char*)(username ~ ":" ~ password)
because the result of the concatenation is not necessarily
zero-terminated. Please make a pass through all cast instances making
sure the code appends a \0.
* Some lines are long enough to not be visible in git's code viewer.
* More buffer overflow opportunities, e.g. in line 1798: "if
(header[0..5] == "HTTP/")" should be "if (header.startsWith("HTTP/"))"
Andrei
