== Quote from jdrewsen (jdrewsen@nospam.com)'s article
> 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

Ok, this makes more sense if we define the etc package to be wrappers and binding for non-D libraries.  I thought this thing was going into std, in which case it should feel, well, standard.  Is that officially how etc is defined?

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

Will fix

> 2) Some links in the doc says 'std.curl....'

I know. This is a limitation of the ddoc config file. I guess I have to make a patch for that one as well.

Thanks
Jonas

Den 16-08-2011 20:49, dsimcha skrev:
> == Quote from jdrewsen (jdrewsen@nospam.com)'s article
>> 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
>
> Ok, this makes more sense if we define the etc package to be wrappers and binding
> for non-D libraries.  I thought this thing was going into std, in which case it
> should feel, well, standard.  Is that officially how etc is defined?

I don't think there is an official definition yet. But I would like to suggest the setup described to be official one.

On Tue, 16 Aug 2011 20:48:51 +0200, jdrewsen <jdrewsen@nospam.com> wrote:

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

Pardon the typo. What I meant is that AFAIK casting from immutable to mutable has undefined behavior.
The intended method for sending a uint[] buffer to another thread is to cast that
buffer to shared (cast(shared(uint[])) and casting away the shared on the receiving side.
It is allowed to send shared data using std.concurrency.

martin

Den 16-08-2011 15:54, Alix Pexton skrev:
> 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..."

ok

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

The first collected example? Can you tell me the comment above the example you do not understand?

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

This is how it is for all modules AFAIK.

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

ok

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

ok

> Protocol.onProgress represents transfer statistics using doubles?

That is what we get from libcurl

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

I will include a link to the HTTP RFC that explains it.

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

Added explanation and a link to HTTP RFC explaining sematics.

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

ok

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

ok

> Also, later examples are just marked as "Asynchronous version
> of..." and lose the note about callbacks, which I assume still apply.)

The latter examples are trivial HTTP methods not sending or receiving anything but the status codes are used. Therefore they do not need an elaborate example as for the GET/POST/PUT methods.

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

It is possible to provide read functionallity of the properties. The only issue with this is that the delegate read would not be the same as the one just written because it gets wrapped it the propery set call. I don't know if this is a problem though?

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

That's a bug. Will fix.

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

ok

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

That ddoc standard config needs a patch to support etc module xrefs.

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

I will try to improve the docs. They return the EncodingSchemes that is used when reading the data

http://www.d-programming-language.org/phobos/std_encoding.html

The scheme is changed by using the encodingName property.

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

Maybe you're right. Whether it is a product of being a wrapper around an existing library and thereby forcing some design decisions or by my coding style I don't know.

If anyone can give some input as how to make it more D'ish please do.

Thank you the valuable feedback
/Jonas

> A...
>

Den 16-08-2011 18:38, Nick Sabalausky skrev:
> "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".

ok

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

ok

> Also, little bikeshedding, but I think this would be better still:
>
> "Performs an asynchronous HTTP _WHATEVER_ on the specified URL."
>
>

Den 16-08-2011 16:43, Alix Pexton skrev:
> 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.

you mean this one:

client.onReceive = (ubyte[] data) { writeln("Got data", cast(char[]) data); return data.length;};

I cannot identify the problem?

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

A fix to the std ddoc config is needed.

As explained in reply to another post I believe this wrapper should be put in the etc package.

/Jonas

On Tuesday, August 16, 2011 20:23:19 jdrewsen wrote:
> 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?

In general, external libraries should not be included in Phobos. They should be using whatever is on the user's system. As such, you're correct in that etc is probably the place to put the curl wrapper, not std.

- Jonathan M Davis

On Tuesday, August 16, 2011 18:47:44 Jacob Carlborg wrote:
> 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?

No. The style guide says camelcase, but it doesn't specify how to deal with abbreviations. Personally, I think that it should be dealt with on a case by case basis, since what works best depends on the abbreviation, but in general, I prefer all caps.

- Jonathan M Davis

jdrewsen:

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

All necessary binaries of the libraries used by all Phobos modules need to be in the zip of the Windows distribution (and probably all other binary distributions of DMD too). Otherwise they are not worth adding to Phobos.

Bye,
bearophile