Curl wrapper

[Jonas Drewsen]

On 17/05/11 08.03, Robert Jacques wrote:
> 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?

I define a pragma yes.

> Regarding the First example:
> * Use string instead of const(char)[]
Please see my reply to Andrei

> * Don't waste vertical space with empty comment lines like this
ok

> //
> // 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
ok

> * Don't use 'l' as a variable. It looks like a 1. Also, using 'line'
> would make the meaning clearer
ok

> * Example code should not exceed 80 chars in width. People's browser
> setups vary, and wrapped lines are bad.
ok

> * You can drop the 'delegate size_t' from the PUT example by making the
> return 0u, 0U or size_t.init.
nice. will do.

> * The last example is missing a description. (Also, it goes over 80 chars)
ok

> 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)
I'll mention what license libcurl is under. Until a standard for a 
special string/tuple has been decided upon I'll leave this out.

> '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.
I'll do a spell check. See reply to Andrei on the other issue.

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

I don't know what ditto does. I'll try to look at the ddoc documentation.

> 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.
See reply to Andrei.

> The setTimeCondition documentation seems a bit confusing.
I'll try to improve.

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

As mentioned I need to figure out what ditto does.

headAsync comment does not match its signature. I'll fix.

I've noticed that trying out examples live in the docs may be activated 
at some point. If that is the case I think it is nice to have running 
examples of each method.

> Check the grammar in http.postData
ok

> 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;
> }
>

I see your point. The question is how we would like phobos docs to be:

1, consistent with all parameters documented for each method and each 
method explained and exemplified
2, minimal with examples and explanations where absolutely needed.

or somewhere in between. The current style if the curl wrapper is closer 
to 1 than 2.

Anything official about this?


> 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:
> */
ok

> You need to fix/complete onSend's documentation.
Will give it a look

> contentLength(size_t len); => contentLength(size_t length);
>
> 'Perform http request' => 'Perform an http request'
ok

> AuthMethod/CurlAuth need to be part of docs.
I guess it I should alias the enum in etc.curl directly

> Why is setAuthenticationMethod not authenticationMethod or
> authentication? (And why is AuthMethod not AuthenticationMethod?)
> Similarly, why is setTimeCondition not timeCondition and CurlTimeCond
> not Http.TimeCondition?
authenticationMethod will be made into a property.
AuthMethod because that's what it is called in libcurl. I like the short 
form better and have done the same for AsyncHttpResult (not 
AsynchronousHttpResult).

setTimeCondition cannot be made into a property since it takes two 
arguments.

CurlTimeCond is not calld Http.TimeCondition because it comes from a 
etc.c.curl. I could alias it in Http ofcourse.

> Why is followLocation(int maxRedirs) not named maxRedirections?

This is what it is called in libcurl. I think I'll rename it to 
maxRedirections anyway since it seems to confuse.

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

It is mostly based on storage concerns. Initially the data is stored as 
ubyte[] and can be returned by content(). When calling text() to get a 
string it will be decoded and returned. I now have a coule of options:

1, to store the string in the result for future calls to text(). I can 
do that and keep the original data for future calls to content(). This 
will double the amount of memory needed.
2, generated the string on each call to text(). This way no copy of the 
data is kept in HttpResult.
3, generated and store the string and then throw away the original data. 
Again no copy is kept in HttpResult.

I've chosen the 2. version since I guessed that once you have called 
text() it is more likely that you'll call text again and not content().

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

> Why is HttpStatusLine not Http.Status?


Thank you for your feedback!