www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - etc.curl: Review ends today

reply David Nadlinger <see klickverbot.at> writes:
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
Aug 31 2011
next sibling parent dsimcha <dsimcha yahoo.com> writes:
On 9/1/2011 12:43 AM, David Nadlinger wrote:

 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.


The std.parallelism review process kind of ended up the same way, by 
accident/evolution instead of design.  I think this is a good 
modification to our review process:  A 2-3 week initial review, then 
instead of an immediate vote the review manager may postpone the vote 
until after the next review queue item.  This should happen if major 
changes are suggested and gives the author time to make changes.  Then, 
have a one-week combined final review/vote as you suggest.

One of the things that I found frustrating in the std.parallelism review 
process was that a lot of (very good) suggestions came in late in the 
review process when I felt like I had to rush to get them implemented in 
time for the vote.  The more we can mitigate this, the better.
Aug 31 2011
prev sibling next sibling parent Jonathan M Davis <jmdavisProg gmx.com> writes:
On Thursday, September 01, 2011 06:43:46 David Nadlinger wrote:

 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.


Considering the massive changes that are likely to be required for the module 
based on the review, I think that it's clearly going to require another review 
phase prior to a vote, and it may be that we'll want a shorter review period 
for the second review, but I would point out that it's not necessarily a good 
idea to try and push a vote too quickly if enough changes have to be made. 
It's better IMHO to have multiple review periods as the issues with the module 
are sorted out rather than forcing a vote before the module is really ready 
for it. But we'll obviously have to see where the module stands when it's 
reviewed next before we decide whether it's ready for voting or not.

- Jonathan M Davis
Sep 01 2011
prev sibling next sibling parent reply jdrewsen <jdrewsen nospam.com> writes:
Den 01-09-2011 06:43, David Nadlinger skrev:
[snip]


 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.


I have done a lot of the changes but there are still quite some work 
left to do. Please go on with another review and I'll get back when I'm 
ready for a next round.


 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.


Sounds good to me.

Thanks for running this review.

/Jonas
Sep 01 2011
next sibling parent David Nadlinger <see klickverbot.at> writes:
On 9/1/11 6:28 PM, jdrewsen wrote:

 I have done a lot of the changes but there are still quite some work
 left to do. Please go on with another review and I'll get back when I'm
 ready for a next round.


Okay, waiting for a ping from you then.


 Thanks for running this review.


The pleasure was all mine. ;)

David
Sep 01 2011
prev sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2011-09-01 18:28, jdrewsen wrote:

 Den 01-09-2011 06:43, David Nadlinger skrev:
 [snip]


 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.


 I have done a lot of the changes but there are still quite some work
 left to do. Please go on with another review and I'll get back when I'm
 ready for a next round.


 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.


 Sounds good to me.

 Thanks for running this review.

 /Jonas


BTW, shouldn't etc.curl be std.net.curl now when we have a std.net package.

-- 
/Jacob Carlborg
Sep 01 2011
parent reply Damian Ziemba <nazriel driv.pl> writes:
On Thu, 01 Sep 2011 20:00:46 +0200, Jacob Carlborg wrote:


 On 2011-09-01 18:28, jdrewsen wrote:

 Den 01-09-2011 06:43, David Nadlinger skrev: [snip]


 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.


 I have done a lot of the changes but there are still quite some work
 left to do. Please go on with another review and I'll get back when I'm
 ready for a next round.


 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.


 Sounds good to me.

 Thanks for running this review.

 /Jonas


 
 BTW, shouldn't etc.curl be std.net.curl now when we have a std.net
 package.


In general splitting Phobos into subpackages would be great idea.
Now everything goes to root, if somebody implements more digest 
mechanisms we will have in root, std.md5, std.sha256, std.sha1, 
std.whirpool etc etc, std.digest.X looks way much better. Same goes with 
std.datetime, it really could be splitted into std.date.time, std.date.UTC 
etc etc :-D

Sorry for off-topic :-p
Sep 01 2011
parent Jacob Carlborg <doob me.com> writes:
On 2011-09-02 00:23, Damian Ziemba wrote:

 On Thu, 01 Sep 2011 20:00:46 +0200, Jacob Carlborg wrote:


 On 2011-09-01 18:28, jdrewsen wrote:

 Den 01-09-2011 06:43, David Nadlinger skrev: [snip]


 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.


 I have done a lot of the changes but there are still quite some work
 left to do. Please go on with another review and I'll get back when I'm
 ready for a next round.


 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.


 Sounds good to me.

 Thanks for running this review.

 /Jonas


 BTW, shouldn't etc.curl be std.net.curl now when we have a std.net
 package.


 In general splitting Phobos into subpackages would be great idea.
 Now everything goes to root, if somebody implements more digest
 mechanisms we will have in root, std.md5, std.sha256, std.sha1,
 std.whirpool etc etc, std.digest.X looks way much better. Same goes with
 std.datetime, it really could be splitted into std.date.time, std.date.UTC
 etc etc :-D

 Sorry for off-topic :-p


I agree that std.datetime should be splitted into several modules living 
in a new sub package.

-- 
/Jacob Carlborg
Sep 01 2011
prev sibling parent reply Don <nospam nospam.com> writes:
On 01.09.2011 06:43, David Nadlinger wrote:

 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:



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


That's been gone for years. It was removed in DMD 2.027.


 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.
Sep 04 2011
parent David Nadlinger <see klickverbot.at> writes:
On 9/4/11 9:09 PM, Don wrote:

 On 01.09.2011 06:43, David Nadlinger wrote:

 Besides an implementation of the Gamma function,


 That's been gone for years. It was removed in DMD 2.027.


etc/gamma.d is still in Git master.

David
Sep 04 2011