"David Nadlinger" <see@klickverbot.at> wrote in message news:j2e15a$2nsh$1@digitalmars.com...
> 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.
>

Curious: What exactly is the role of the review manager?

"Alix Pexton" <alix.DOT.pexton@gmail.DOT.com> wrote in message news:j2dsq0$2aht$1@digitalmars.com...
>
> "...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."
>

"in the buffer that has been filled" is correct. The "has" refers to "the buffer" not the elements that have been put into it. And there's only one buffer in question, so "has".

But yea, "is ready to send" should be changed to "are ready to send".


> "Asynchronous HTTP _WHATEVER_ to the specified URL." -> "Performs a HTTP _WHATEVER_ on the specified URL, asynchronously."

"Performs a HTTP" -> "Performs an HTTP". The letter "H" may technically be a consonant, but in this case it's read as the *name* of the letter, not the letter's normal sound. And the name of H starts with a vowel sound (long A). (Besides, "a HTTP" just sounds awkward.)

Also, little bikeshedding, but I think this would be better still:

"Performs an asynchronous HTTP _WHATEVER_ on the specified URL."

On 2011-08-16 15:54, kennytm wrote:
> 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....'

BTW, do we have style guides for abbreviations like this?

-- 
/Jacob Carlborg

On Tue, 16 Aug 2011 15:13:40 +0200, dsimcha <dsimcha@yahoo.com> wrote:

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

I was also wondering about the async handling. In the long-term I'd like to see a bigger picture for async handling in phobos (offering some kind of futures, maybe event-loops etc.).
Though this is not a requirement for the curl wrapper now. std.parallelism also has some kind of this stuff and file reading would benefit from it too.

One thing I spotted at a quick glance, sending to be filled buffers to another thread should not be done by casting to shared not immutable.

martin

On Tue, Aug 16, 2011 at 10:08 AM, David Nadlinger <see@klickverbot.at>wrote:

> 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<https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d> Docs: http://freeze.steamwinter.com/**D/web/phobos/etc_curl.html<http://freeze.steamwinter.com/D/web/phobos/etc_curl.html>
>>
>> Demolish!
>>
>> /Jonas
>>
>
>

On line 80, I made a typo when writing the documentation:

https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L80

It should be:
<to.addr@gmail.com>

After reading the libcurl documentation, I realize that the automatic angle bracket attaching at lines 2932 and 2943 are not needed (libcurl seems to do it for you after version 7.21.4).

On Tue, 16 Aug 2011 08:53:12 -0400, bearophile wrote:

> Jonas Drewsen:
> 
>> Demolish!
> 
> I'd like to try this module. Do you have a compiled curl lib for Windows?

http://curl.haxx.se/download.html

-Lars

On Tue, 16 Aug 2011 12:26:19 -0400, Nick Sabalausky wrote:

> "David Nadlinger" <see@klickverbot.at> wrote in message news:j2e15a$2nsh$1@digitalmars.com...
>> 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.
>>
>>
> Curious: What exactly is the role of the review manager?

Andrei has suggested we follow Boost's review model:

http://www.boost.org/community/reviews.html

-Lars

Den 16-08-2011 14:57, Jacob Carlborg skrev:
> 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.

Ahh... thank you for the pointer.

That is only one part of the ddoc issues. The other one is that the Protocol mixin (which is mixed into the Http/Ftp/Smtp classes) does not generate documentation in the Http/Ftp/Smtp classes. I guess that is another bug/feature.

Den 16-08-2011 15:13, dsimcha skrev:
> 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?

It uses the spawn method from std.concurrency which uses D threads i think.

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

Not really. It is just an example and writeln will output a byte array just fine.

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

To specify the number of bytes that have been handled. If this is less that the entire buffer the rest of the request will fail. Additionally both the onSend and onReceive callbacks can return  CURL_WRITEFUNC_PAUSE pr CURL_READFUNC_PAUSE to pause the transfer. And onSend can return CURL_READFUNC_ABORT to abort as well. I see that the is missing from the docs... will add.

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

I think that would be convenient but don't believe it is the correct thing to do. In my opinion D modules that are not pure D or wrapping a system library (which libcurl is definitely not) should be the only ones allowed in the std package. Other wrappers in phobos should be in the etc package and the lib binaries should not be included.

Just how I feel about it though. Anyone who disagrees?

/Jonas

Den 16-08-2011 18:55, Martin Nowak skrev:
> On Tue, 16 Aug 2011 15:13:40 +0200, dsimcha <dsimcha@yahoo.com> wrote:
>
>> 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?
>
> I was also wondering about the async handling. In the long-term I'd like
> to see a bigger picture for async handling in phobos (offering some kind
> of futures, maybe event-loops etc.).
> Though this is not a requirement for the curl wrapper now.
> std.parallelism also has some kind of this stuff and file reading would
> benefit from it too.

This has been discussed before and I also think this is very important. But before that I think some kind of package management should be prioritized (A DIP11 implementaion or a more traditional solution).

> One thing I spotted at a quick glance, sending to be filled buffers to
> another thread should not be done by casting to shared not immutable.

I'm not entirely sure what you mean. There is no use of shared buffers in the wrapper. I do cast the buffer between mutable/immutable because only immutable or by value data can be passed using std.concurrency. Since the buffers are only used by the thread that currently has the buffer this is safe. I've previously asked for a non-cast solution (ie. some kind of move between threads semantic for std.concurrency) but was advised that this was the way to do it.

>
> martin