etc.curl: Formal review begin

[jdrewsen]

Den 29-08-2011 20:55, Andrei Alexandrescu skrev:
> 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.

Others have mentioned this as well. I'll try to improve.

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

I guess I'm assuming that the user knows something about curl. Will have 
a look at it.

> ================
> Documentation
> ================
>
> * Should be std.curl.
ok

> * "Curl client functionality as provided by libcurl." -> "Networking
> client functionality as provided by libcurl."
ok

> * Code inlined within text should use $(D ...).
ok

> * The first example is meaningless:
>
> Http.get("http://www.google.com").connectTimeout(dur!"seconds"(10)).toString();
Someone else mentioned that as well.

> * "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");"
ok

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

I could easily create a convenience Http.byLine("google.com") method 
that simply wraps Http.get("google.com").byLine().
I think that keeping the Http.get("google.com") method is still 
important since it allows you to change the request settings before 
calling byLine(). For example:

Http.get("google.com").connectTimeout(dur!"seconds"(10)).byLine();


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

> * 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.
That example it certainly broken. Anyway - the data is the buffer that 
curl want to be filled with data to send. The length of data is how 
large that buffer is of course.
As specified it is only necessary to set the content length if you do 
not want to used chunked transfer. Most often you wouldn't care.
Maybe I should just remove the contentLength from the example.

> * "Smtp smtp = Smtp("smtps://smtp.gmail.com");" -> "auto ..."
ok

> * "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.
ok

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

> * 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().
agreed

> * verbose, dataTimeout etc. don't have corresponding properties for
> reading.
I general all the settings are simply set directly in libcurl itself. 
Libcurl only has a function for setting options and not one for getting 
them. This means that I would have to keep a copy of the setting myself 
just to make it readable which i judged was not the way to go. This is 
why they are read only. This is one of the reasons why I see a wrapper 
for libcurl as just temporary solution to a native networking library i D.

> * 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.
This is the libcurl API reflected. I've got no problem with overloading 
the netInterface to accept ubyte or an IP struct as well.

> * localPort() should take a ushort, not an int.
Again just reflecting libcurl. But will make the cast to make it cleaner.

> * Phrases such as "tcp nodelay socket option" should link to relevant
> off-site documentation or a glossary entry.
ok

> * onSend example is unindented, uses "l" as a symbol, and is liable to
> buffer overflow, which is, to be brutally honest, appalling.
Agreed. Embarressing.

> * The example for onReceive should use to!(const(char)[]) instead of the
> cast, such that the UTF validity is checked.
ok

> * Since neither value in onProgress can be fractional, the type should
> be size_t or ulong.
You are not the first to mention it. Again it is what the libcurl API 
provides. But I will fix it.

> * Due to probably a bug in ddoc, the members of struct Http are not
> indented.
Noticed. I'll see if it can be worked around.

> * The informal examples given with addHeader and setHeader should also
> be supported by code examples.
ok

> * Example for head() misses a semicolon.
ok

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

> * optionsAsync has an extra paren.
ok

> * The various HTTP primitives wrapped (del, connect, ...) should include
> links to their definition in the documentation.
ok

> * The dox for the two postData() overloads should be merged.
ok

> * 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.
I guess I'll have to move it to the top.

> * Again, many properties have setters but not getters (e.g. maxRedirects).
see comment about "verbose, dataTimeout..."

> * The word "simply" is overused.
ok

> ================
> Implementation
> ================
>
> * We usually import modules in alphabetical order.
ok

> * The frequent pattern if (!expr) throw new CurlException(msg); should
> be replaced by enforce(expr, new CurlException(msg));
ok

> * throwOnStopped should accept a defaulted string. There are several
> instances of the pattern if (stopped) throw new CurlException("...");
ok

> * I think we can in the future replace curl_easy_(un)escape with more
> efficient native functions.
probably. will put it in the "Possible improvements" section in the top 
of the source.

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

> * Some lines are long enough to not be visible in git's code viewer.
I've got a commit ready already with a lot of suggested fixes/changes 
from other review comment. One if these is limiting lines to max 80-120 
char.
When this first review ends on wednesday I'll commit that and begin 
handling the rest of the review comments.

> * More buffer overflow opportunities, e.g. in line 1798: "if
> (header[0..5] == "HTTP/")" should be "if (header.startsWith("HTTP/"))"
ok

> Andrei

Thank you for you comments.