Hi all,

   This is a review request for the curl wrapper. Please read the "known issues" in the top of the source file and if possible suggest a solution.

We also need somebody for running the review process. Anyone?

Code:
	https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d	
Docs:
	http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

Demolish!

/Jonas

On 2011-08-16 13:48, Jonas Drewsen wrote:
> Hi all,
>
> This is a review request for the curl wrapper. Please read the "known
> issues" in the top of the source file and if possible suggest a solution.
>
> We also need somebody for running the review process. Anyone?
>
> Code:
> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
> Docs:
> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>
> Demolish!
>
> /Jonas

Is it just me or is it very annoying that the jump-to section in the documentation shows non-top declarations, e.g. methods, enum members, struct functions and so on.

I know this is not specific to the curl documentation.

-- 
/Jacob Carlborg

Jonas Drewsen:

> Demolish!

I'd like to try this module. Do you have a compiled curl lib for Windows?

Bye,
bearophile

On 2011-08-16 13:48, Jonas Drewsen wrote:
> Hi all,
>
> This is a review request for the curl wrapper. Please read the "known
> issues" in the top of the source file and if possible suggest a solution.
>
> We also need somebody for running the review process. Anyone?
>
> Code:
> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
> Docs:
> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>
> Demolish!
>
> /Jonas

The ddoc issue: http://d.puremagic.com/issues/show_bug.cgi?id=648
I would very much like to have this fixed.

-- 
/Jacob Carlborg

On 8/16/2011 7:48 AM, Jonas Drewsen wrote:
> Hi all,
>
> This is a review request for the curl wrapper. Please read the "known
> issues" in the top of the source file and if possible suggest a solution.
>
> We also need somebody for running the review process. Anyone?
>
> Code:
> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
> Docs:
> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>
> Demolish!
>
> /Jonas

From a quick look, this looks very well thought out.  I'll review it more thoroughly when I have more time.  A few questions/comments from a quick look at the docs:

Does the async stuff use D threads, or does Curl have its own async API?

In your examples for postData, you have onReceive a ubyte[] and write it out to console.  Did you mean to cast this to some kind of string?

For onReceive, what's the purpose of the return value?

If/when this module makes it into Phobos, are we going to start including a libcurl binary with DMD distributions so that std.curl feels truly **standard** and requires zero extra configuration?

On 16/08/2011 12:48, Jonas Drewsen wrote:
> Hi all,
>
> This is a review request for the curl wrapper. Please read the "known
> issues" in the top of the source file and if possible suggest a solution.
>
> We also need somebody for running the review process. Anyone?
>
> Code:
> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
> Docs:
> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>
> Demolish!
>
> /Jonas


Here are my initial observations and impressions...

In the opening description...
"The request is perform when first accessing..." -> "The request is performed when first accessing..."

On first reading, I found it very hard to follow the first collected example. (Still not 100% clear after reading the whole of the documentation either >< )

Some (all?) of the @property members of Protocol are documented as functions. (Perhaps an artefact of the ddoc mixin issue mentioned before this section?)

Description of Protocol.onSend...
"...specifies the max number of bytes that can be send." -> "...specifies the maximum number of bytes that can be sent."
"...returns the number of elements in the buffer that has been filled and is ready to send." -> "...returns the number of elements in the buffer that have been filled and are ready to send."

Description of Protocol.onReceive...
"...is not guaranteed to be valid aften the callback returns." -> "...is not guaranteed to be valid after the callback returns."

Protocol.onProgress represents transfer statistics using doubles?

CurlTimeCond, members are all lower case, took me a few goes to work out what they might mean, still not all that sure. Should I assume that if the meanings are not obvious that I'm not ready to be tinkering with curl at this level, or should each value be clarified?

Http.addHeader / Http.setHeader both have the same description, I assume the latter overwrites an existing value if it exists? what is the behaviour of addHeader when the key already exists?

Http.setTimeCondition parameter names in ddoc don't match those in the function prototype.

Http._whatever_Async (and also Async functions for Ftp.)
"Asynchronous HTTP _WHATEVER_ to the specified URL." -> "Performs a HTTP _WHATEVER_ on the specified URL, asynchronously."
(Wording looks very similar across this group of functions, not sure if my suggested alteration here correctly captures the subtleties for all cases. Also, later examples are just marked as "Asynchronous version of..." and lose the note about callbacks, which I assume still apply.)

All callbacks are write only properties which prevents the use of event hooking (which, in my experience, is a common occurrence in event based models). Hooking might not apply in this case however (I've not particularly thought through any use cases) and safe event hooking might be beyond the scope of this module.

Http.traceAsync
Calls AsyncResult with Method.get? I would have expected Method.trace, but I only spotted this by chance, could very well be my ignorance showing ^^

Http.onReceiveStatusLine
"...several callbacks can be done for on call to perform() because if redirections." -> "...several callbacks can be done for each call to perform() [because of | due to] redirections."

Http.authenticationMethod
std.etc.c.curl.AuthMethod? I think this is an issue with the XREF macro not expecting the link to break out of the std package.

Ftp.Result.encoding / Ftp.AsyncResult.encoding
I'm not sure what these functions are supposed to return, the descriptions seem inadequate.

---

Not fully read all the source yet, but as I'm not all that familiar with curl, I'm not sure I'll spot much ^^

Not a criticism, but at first glance, it doesn't feel very D-ey, but I'm still not 100% sure what idiomatic D is yet.

A...

Jonas Drewsen <jdrewsen@nospam.com> wrote:
> Hi all,
> 
>    This is a review request for the curl wrapper. Please read the "known
> issues" in the top of the source file and if possible suggest a solution.
> 
> We also need somebody for running the review process. Anyone?
> 
> Code:
> 	https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
> Docs:
> 	http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
> 
> Demolish!
> 
> /Jonas

1) Why 'Http' and 'Ftp' but 'SMTP' is in all caps?
2) Some links in the doc says 'std.curl....'

Alix Pexton <alix.DOT.pexton@gmail.DOT.com> wrote:

> 
> Some (all?) of the @property members of Protocol are documented as
> functions. (Perhaps an artefact of the ddoc mixin issue mentioned before this section?)
>

Bug 3445.

> Protocol.onProgress represents transfer statistics using doubles?
>

Unfortunately curl's API uses double.

On 16/08/2011 12:48, Jonas Drewsen wrote:
> Hi all,
>
> This is a review request for the curl wrapper. Please read the "known
> issues" in the top of the source file and if possible suggest a solution.
>
> We also need somebody for running the review process. Anyone?
>
> Code:
> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
> Docs:
> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>
> Demolish!
>
> /Jonas

Spotted another issue...

In the example for Protocol.onReceive, there is a mismatch between the parameter names specified for the delegate and those used inside it.

Also, all the XREF macros point to std_curl.html. instead of etc_curl.html, is this going to become std.curl when incorporated into Phobos?

A...

Seems like the reviews are already coming in steadily, but we still have no review manager – if nobody else steps up until tomorrow, I'd volunteer so we can get the formal review process going.

David


On 8/16/11 1:48 PM, Jonas Drewsen wrote:
> Hi all,
>
> This is a review request for the curl wrapper. Please read the "known
> issues" in the top of the source file and if possible suggest a solution.
>
> We also need somebody for running the review process. Anyone?
>
> Code:
> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
> Docs:
> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>
> Demolish!
>
> /Jonas