Curl wrapper

[Robert Jacques]

On Mon, 16 May 2011 17:07:57 -0400, jdrewsen <jdrewsen at nospam.com> wrote:

> Hi,
>
>     I've been working on a wrapper for the etc.c.curl module. It is now  
> pretty stable and I would very much like some feedback on the API.
>
> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>
> BTW I use template mixins which doesn't seem to get included in the  
> generated docs. Is there any way I can make this work?
>
> /Jonas

I think the API looks good with regard to the core functionality it  
provides, but the documentation is sub-par, an certain structs should be  
defined inside the Http class. I've listed a bunch of comments I had while  
reading the documentation below.

One minor question regarding the actual implementation: do you define a  
pragma(lib, ...) to ease setup/tool chain pains, or do you intend  
something different?

Regarding the First example:
  * Use string instead of const(char)[]
  * Don't waste vertical space with empty comment lines like this
//
// This comment style
//
  * Comment lines should be grammatically correct, i.e. '// GET using async  
range' should be '// GET using an asynchronous range' or '// Perform a GET  
using an asynchronous range', etc
  * Don't use 'l' as a variable. It looks like a 1. Also, using 'line'  
would make the meaning clearer
  * Example code should not exceed 80 chars in width. People's browser  
setups vary, and wrapped lines are bad.
  * You can drop the 'delegate size_t' from the PUT example by making the  
return 0u, 0U or size_t.init.
  * The last example is missing a description. (Also, it goes over 80 chars)

The license section should include a mention of libcurl's license, etc in  
addition to the wrapper's license. In general, for non-Boost libraries  
like this, should we include the license/copyright as a string/tuple enum?  
(i.e. for use in about boxes / command line options) Should said  
license/copyright be register to a global repository upon module import?  
(i.e. for auto-magical about boxes, etc)

'Do not use the same instance of this class in two threads simultanously.'  
First, you need to spell check all your documentation (i.e. simultaneously  
vs simultanously). Second, this should be assumed by most programmers to  
be true of all non-shared classes. Third, if you do want to leave this  
line in, use something short and sweet, like 'Http is not thread safe',  
instead.

You need to look at/think about the order in which things are declared.  
Having things like HttpMethod declared long after Http doesn't make any  
sense. Also, it looks like you're not using '///ditto' for HttpMethod.

On that note, why is HttpMethod a free-standing enum and not declared  
inside http? Http.Method, Http.Result, etc. make a lot more sense.

The setTimeCondition documentation seems a bit confusing.

I'd recommend stream-lining the convenience function documentation. For  
example, http.head only requires a /// Returns: An HttpResult containing  
the url's headers. Alternatively, you could use ditto and merge the  
documentation and examples for head/get/etc into a single block. Also,  
headAsync seems to document a data argument, but not take one, while post  
has an undocumented parameter, etc. In general, you might want to consider  
removing some of the redundant/self-obvious parameter documentation  
blocks. On second thought, use ditto, slim down the docs and move this set  
of functions either to the beginning or end of Http. It would make sense  
to put these right after the definition of Http.Method/method. You also  
might want to consider grouping all the xxxAsync functions together,  
instead of interleaving them.

Check the grammar in http.postData

http.onReceiveHeader has issues with it's 'See' link. It's example is too  
wide. You don't need really need the parameter documentation, particularly  
if you improve the example, i.e.:
string[string] headerInformation;
with(auto http = new Http("http://www.google.com")) {
     onReceiveHeader = (string key, string value) {
         if ( value.length <= 10 )
             headerInformation[key.idup] = value.idup;
     };
     onReceive = (ubyte[] data) { };
     perform;
}


Also, an tab/indent should be 4 spaces not 5 and no example should start  
indented. Remember to comment like this:
          /** My regular comment
           *  Example:
---
// Example code
---
           * Other Stuff:
           */

You need to fix/complete onSend's documentation.

contentLength(size_t len); => contentLength(size_t length);

'Perform http request' => 'Perform an http request'

AuthMethod/CurlAuth need to be part of docs.

Why is setAuthenticationMethod not authenticationMethod or authentication?  
(And why is AuthMethod not AuthenticationMethod?)
Similarly, why is setTimeCondition not timeCondition and CurlTimeCond not  
Http.TimeCondition?

Why is followLocation(int maxRedirs) not named maxRedirections?

HttpResult.text should have huge warning flags with regard to use and/or  
have its design re-thought. Is there some reason you're not at least  
caching the result and throwing errors on invalid uses of content, etc.  
Calling text multiple times should be okay. And calling content after text  
should be either logically valid, or throw.

AsyncHttpResult.byLine/byChunk examples is improperly indented and too  
wide.

Why is HttpStatusLine not Http.Status?