etc.curl: Review ends today

[David Nadlinger]

I forgot to specify a timezone in the announcement, so depending on your 
location the code review period for the etc.curl module by Jonas Drewsen 
has either already ended, or is going to end later today:

Thanks to everybody who took the time to review the library; no matter 
how the vote will turn out, your suggestions will have helped a large 
amount on the way to a comprehensive URL transfer library for the D 
standard library. Let me try to quickly summarize the discussion, 
ignoring comments about smaller implementation details and stylistic issues:

  - Concerns were raised that the removal of the libcurl built-in 
progress callback implementation (on the libcurl TODO list, and 
mentioned in the »possible improvements« section in etc.curl) might 
affect the etc.curl API, but this was merely a misunderstanding.

  - The library doesn't provide URL encoding/decoding functionality, but 
we have std.uri for that.

  - A convenience interface for just getting the contents of an URL 
without manually specifying the protocol (file://, http://, …) either as 
a byte array/string or a range of chunks/lines would be a welcome 
addition, as this covers the common use cases of just needing to 
download some file.

  - When using Http/Ftp/Smtp to create a request, it should be allowed 
to omit the scheme part of the URL.

  - The names of the static convenience functions for the Http verbs and 
their …Async() variants do not reflect that they actually prepare 
request which are not immediately executed. Combined with the fact that 
the returned struct types bear »Result« in their name and, more 
importantly, that D allows to call static methods using the dot syntax 
on an instance, this makes it easy to run into a trap by writing 
something like »auto client = Http("google.com/search"); … add headers, 
etc. … client.post();«.

  - The type of several fields/parameters is not optimal: only string 
for IP addresses, int instead of ushort for ports, double for the number 
of bytes in the progress callback. This comes from mirroring the Curl 
API, but can be changed.

  - Properties like verbose, dataTimeout, … only have setters, getters 
are missing due to a libcurl limitation. Providing read access would 
require keeping shadow copies of all the values, so it will not be added.

  - Proxy support (CURLOPT_PROXY, CURLOPT_PROXYPORT, CURLOPT_PROXYTYPE) 
is currently missing from the library.

  - The API doesn't use any of the »advanced« attributes like 
safe/trusted, pure, nothrow, …

  - The async request implementation casts mutable buffer slices to 
immutable and back again to be able to pass them via std.concurrency. 
While currently safe in practice, it was argued that this relies on 
undefined behavior, since writing to data cast from immutable would be 
disallowed regardless of the »original« mutability. The spec can be 
interpreted in two different ways here, and the resulting discussion did 
not lead to a clear conclusion.
    Using »shared« seems like a cleaner solution, but isn't possible due 
to issue 6585. [1]

  - Line length should be limited to about 80 characters, and shorter 
lines are to be greatly preferred for doc examples. We need to add a 
guideline for the latter to the D style guide.

  - Documentation for mixin templates members not being generated (bug 
648 [2]) is an unsolved problem, and it affects the library 
documentation quite badly for the (Async)Result structs. Furthermore, 
the members of several structs (Http, Ftp, nested (Async)Result structs) 
are not indented, which is probably a DDoc bug.

  - The documentation should use cross-references, and additionally 
provide link to some (probably external) glossary for uncommon terms 
like TCP_NODELAY/Nagle's algorithm.

  - The examples need to be improved: Simple things should be shown 
first, they should be as concise as possible (using auto, not 
duplicating the protocol type used, …), a RESTful service query example 
would be nice. Some of the examples contain buffer overflows bugs. A 
separate article covering several advanced use cases in-depth would be a 
good idea.

  - Finally, it is not quite clear what the best place for this library 
is. Currently, it resides in the »etc« package because it depends on an 
external C library, but this decision has been questioned:
    To begin with, it is not clear what the »etc« package is for in the 
first place. The Phobos documentation index [3] only has »Modules in etc 
are not standard D modules. They are here because they are experimental, 
or for some other reason are not quite suitable for std, although they 
are still useful«. Besides an implementation of the Gamma function, the 
package currently contains translated C headers for zlib and sqlite, so 
there is precedent for the aforementioned position that etc is the 
appropriate place for modules depending on external libraries.
    On the other hand, it has been compellingly argued that 
communicating over e.g. HTTP is a very common task nowadays, and Phobos 
should provide an out of the box solution for this. Andrei, among 
others, proposed to move the curl module to the »std« package. This 
would also mean including a libcurl binary with the DMD/Phobos download 
for Windows (on the other platforms, it is commonly available as a 
system-wide installation).
    Concerns have been raised that this would unduly increase the 
download size for users of other platforms (by < ~1 MB), which could be 
mitigated by splitting the DMD download into a source package and 
several binary packages for the various platforms (as suggested several 
times before). Another alternative would be to just provide a link to a 
binary in the documentation instead of redistributing it.


Jonas, do you already have a revised version ready that could 
immediately be voted upon? I recognize that quite a large number of 
suggestions was raised, but as there are quite a few other components 
currently waiting in the review queue, we want to make sure not to 
introduce any avoidable delays while working through that list.

If not, I would propose to postpone the voting phase and start the 
review for another component, e.g. David Simcha's std.regionallocator, 
now. After Jonas had enough time to finish a version he considers fit 
for inclusion into Phobos, I would suggest a short (one week) combined 
review/vote period to decide on inclusion with the standard library.

David


[1] http://d.puremagic.com/issues/show_bug.cgi?id=6585
[1] http://d.puremagic.com/issues/show_bug.cgi?id=648
[2] http://d-programming-language.org/phobos/index.html