Hi,

   I've finally got through all the very constructive comments from the last review of the curl wrapper and performed the needed changes.

Here is the github branch:
https://github.com/jcd/phobos/tree/curl-wrapper

And the generated docs:
http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

I do have some problems getting ddoc to show the documentation of mixins. So in order to view the doc for byLine/byChunk methods you have to look at the source.

Anyway...this is what I've been up to:

New features:

* Full support for async/sync by line/chunk
* FTP support extended from only allowing download of a file sync into full async/sync by line/chunk support
* Allow providing parameters such as credentials/timeouts when using the convenience statis methods.

Changes caused by last review:

* rethink byLine/... to not return string in order to prevent allocations. they should return char[]/ubyte[]
* 80 chars
* Http.Result not HttpResult
* gramma for http.postData
* len -> length
* perform http request -> perform a http ...
* authMethod to property
* curltimecond alias into module
* followlocation -> maxredirs
* http not class anymore but struct
* timecondition use std.datetime
* timeouts use core.duration
* Spelling "callbacks is not supported"
* refer to HTTP RFC describing the methods
* login/password example
* chuncked -> chunked
* max redirs; use uint.max and not -1
* isRunning returining short
* 4 chars tabs in examples.
* no space in examples.
* Send/recv use special structs in order not to mess with other communications

Comments are welcome.

/Jonas

Would an SMTP protocol struct be beneficial?

This looks great, thanks for you work.

On Sat, Jun 18, 2011 at 3:36 PM, jdrewsen <jdrewsen@nospam.com> wrote:

> Hi,
>
>   I've finally got through all the very constructive comments from the last
> review of the curl wrapper and performed the needed changes.
>
> Here is the github branch: https://github.com/jcd/phobos/**tree/curl-wrapper<https://github.com/jcd/phobos/tree/curl-wrapper>
>
> And the generated docs: http://freeze.steamwinter.com/**D/web/phobos/etc_curl.html<http://freeze.steamwinter.com/D/web/phobos/etc_curl.html>
>
> I do have some problems getting ddoc to show the documentation of mixins. So in order to view the doc for byLine/byChunk methods you have to look at the source.
>
> Anyway...this is what I've been up to:
>
> New features:
>
> * Full support for async/sync by line/chunk
> * FTP support extended from only allowing download of a file sync into full
> async/sync by line/chunk support
> * Allow providing parameters such as credentials/timeouts when using the
> convenience statis methods.
>
> Changes caused by last review:
>
> * rethink byLine/... to not return string in order to prevent allocations.
> they should return char[]/ubyte[]
> * 80 chars
> * Http.Result not HttpResult
> * gramma for http.postData
> * len -> length
> * perform http request -> perform a http ...
> * authMethod to property
> * curltimecond alias into module
> * followlocation -> maxredirs
> * http not class anymore but struct
> * timecondition use std.datetime
> * timeouts use core.duration
> * Spelling "callbacks is not supported"
> * refer to HTTP RFC describing the methods
> * login/password example
> * chuncked -> chunked
> * max redirs; use uint.max and not -1
> * isRunning returining short
> * 4 chars tabs in examples.
> * no space in examples.
> * Send/recv use special structs in order not to mess with other
> communications
>
> Comments are welcome.
>
> /Jonas
>
>

Also, why the bool dummy argument in the Curl struct constructor?

On Sat, Jun 18, 2011 at 3:52 PM, Jimmy Cao <jcao219@gmail.com> wrote:

>
>
> On Sat, Jun 18, 2011 at 3:36 PM, jdrewsen <jdrewsen@nospam.com> wrote:
>
>> Hi,
>>
>>   I've finally got through all the very constructive comments from the
>> last review of the curl wrapper and performed the needed changes.
>>
>> Here is the github branch: https://github.com/jcd/phobos/**tree/curl-wrapper<https://github.com/jcd/phobos/tree/curl-wrapper>
>>
>> And the generated docs: http://freeze.steamwinter.com/**D/web/phobos/etc_curl.html<http://freeze.steamwinter.com/D/web/phobos/etc_curl.html>
>>
>> I do have some problems getting ddoc to show the documentation of mixins. So in order to view the doc for byLine/byChunk methods you have to look at the source.
>>
>> Anyway...this is what I've been up to:
>>
>> New features:
>>
>> * Full support for async/sync by line/chunk
>> * FTP support extended from only allowing download of a file sync into
>> full async/sync by line/chunk support
>> * Allow providing parameters such as credentials/timeouts when using the
>> convenience statis methods.
>>
>> Changes caused by last review:
>>
>> * rethink byLine/... to not return string in order to prevent allocations.
>> they should return char[]/ubyte[]
>> * 80 chars
>> * Http.Result not HttpResult
>> * gramma for http.postData
>> * len -> length
>> * perform http request -> perform a http ...
>> * authMethod to property
>> * curltimecond alias into module
>> * followlocation -> maxredirs
>> * http not class anymore but struct
>> * timecondition use std.datetime
>> * timeouts use core.duration
>> * Spelling "callbacks is not supported"
>> * refer to HTTP RFC describing the methods
>> * login/password example
>> * chuncked -> chunked
>> * max redirs; use uint.max and not -1
>> * isRunning returining short
>> * 4 chars tabs in examples.
>> * no space in examples.
>> * Send/recv use special structs in order not to mess with other
>> communications
>>
>> Comments are welcome.
>>
>> /Jonas
>>
>>
>

Den 18-06-2011 22:52, Jimmy Cao skrev:
> Would an SMTP protocol struct be beneficial?

My immediate goal is to provide HTTP support and basic FTP support through libcurl. I believe these are the most important protocols to get in place in order to improve the adoption of D.

I have currently no plans of adding more protocols to the curl wrapper. Patches are welcome :)

I would rather do some work on native async net support since I believe that would give better performance.

/Jonas


> This looks great, thanks for you work.
> On Sat, Jun 18, 2011 at 3:36 PM, jdrewsen <jdrewsen@nospam.com
> <mailto:jdrewsen@nospam.com>> wrote:
>
>     Hi,
>
>        I've finally got through all the very constructive comments from
>     the last review of the curl wrapper and performed the needed changes.
>
>     Here is the github branch:
>     https://github.com/jcd/phobos/__tree/curl-wrapper
>     <https://github.com/jcd/phobos/tree/curl-wrapper>
>
>     And the generated docs:
>     http://freeze.steamwinter.com/__D/web/phobos/etc_curl.html
>     <http://freeze.steamwinter.com/D/web/phobos/etc_curl.html>
>
>     I do have some problems getting ddoc to show the documentation of
>     mixins. So in order to view the doc for byLine/byChunk methods you
>     have to look at the source.
>
>     Anyway...this is what I've been up to:
>
>     New features:
>
>     * Full support for async/sync by line/chunk
>     * FTP support extended from only allowing download of a file sync
>     into full async/sync by line/chunk support
>     * Allow providing parameters such as credentials/timeouts when using
>     the convenience statis methods.
>
>     Changes caused by last review:
>
>     * rethink byLine/... to not return string in order to prevent
>     allocations. they should return char[]/ubyte[]
>     * 80 chars
>     * Http.Result not HttpResult
>     * gramma for http.postData
>     * len -> length
>     * perform http request -> perform a http ...
>     * authMethod to property
>     * curltimecond alias into module
>     * followlocation -> maxredirs
>     * http not class anymore but struct
>     * timecondition use std.datetime
>     * timeouts use core.duration
>     * Spelling "callbacks is not supported"
>     * refer to HTTP RFC describing the methods
>     * login/password example
>     * chuncked -> chunked
>     * max redirs; use uint.max and not -1
>     * isRunning returining short
>     * 4 chars tabs in examples.
>     * no space in examples.
>     * Send/recv use special structs in order not to mess with other
>     communications
>
>     Comments are welcome.
>
>     /Jonas
>
>

On Sat, Jun 18, 2011 at 4:16 PM, jdrewsen <jdrewsen@nospam.com> wrote:

> Den 18-06-2011 22:52, Jimmy Cao skrev:
>
>  Would an SMTP protocol struct be beneficial?
>>
>
> My immediate goal is to provide HTTP support and basic FTP support through libcurl. I believe these are the most important protocols to get in place in order to improve the adoption of D.
>
> I have currently no plans of adding more protocols to the curl wrapper. Patches are welcome :)
>
>
Here's a crude implementation for supporting the SMTP protocol: https://gist.github.com/1033711

What do you think?
My biggest concern is whether SMTP protocol support would really be
necessary within this module.

jdrewsen wrote:
>Hi,
>
>    I've finally got through all the very constructive comments from
> the
>last review of the curl wrapper and performed the needed changes.
>
>Here is the github branch: https://github.com/jcd/phobos/tree/curl-wrapper
>
>And the generated docs: http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>
>I do have some problems getting ddoc to show the documentation of mixins. So in order to view the doc for byLine/byChunk methods you have to look at the source.

That's bad because lots of useful stuff hides in the protocol mixin. The url property for example is essential for keep-alive requests, but it doesn't show up in the documentation :-(

Also, a keep alive example would be great:
--------------------------------------------
auto client = Http("http://api.vevo.com/mobile/v2/authentication.json");
client.addHeader("User-Agent", "Android API Connector");
client.addHeader("Connection", "Keep-Alive");
client.method = Http.Method.post;
client.onReceive = (ubyte[] data) { write(cast(char[])data); return
data.length; };
client.postData = "p=android&v=1.05";
client.perform();

//2nd request
client.url = "http://api.vevo.com/mobile/v1/featured/carousel.json";
client.method = Http.Method.get;
client.perform();
--------------------------------------------
Maybe something like this. (+points if the code uses existing
websites)

BTW: The curl verbose output is great. I guess it won't
be activated in phobos by default, but is it possible to activate it
manually? If so, this very useful feature should be documented.

-- 
Johannes Pfau

Jimmy Cao wrote:
>Also, why the bool dummy argument in the Curl struct constructor?
>

I guess that's because structs can't have default constructors. Is there a better solution to this problem?
-- 
Johannes Pfau

Den 19-06-2011 11:12, Johannes Pfau skrev:
> Jimmy Cao wrote:
>> Also, why the bool dummy argument in the Curl struct constructor?
>>
>
> I guess that's because structs can't have default constructors. Is
> there a better solution to this problem?

That is why yes. I could just make a static "create()" function instead, but I went for this model. I would very much like to know a better solution.

Den 19-06-2011 11:08, Johannes Pfau skrev:
> jdrewsen wrote:
>> Hi,
>>
>>     I've finally got through all the very constructive comments from
>> the
>> last review of the curl wrapper and performed the needed changes.
>>
>> Here is the github branch:
>> https://github.com/jcd/phobos/tree/curl-wrapper
>>
>> And the generated docs:
>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>
>> I do have some problems getting ddoc to show the documentation of
>> mixins. So in order to view the doc for byLine/byChunk methods you
>> have to look at the source.
>
> That's bad because lots of useful stuff hides in the protocol mixin.
> The url property for example is essential for keep-alive requests, but
> it doesn't show up in the documentation :-(

I agree. And also in the ByLineAsync etc. mixins. I would very much like to get a hint on how to do it if anyone knows.

> Also, a keep alive example would be great:
> --------------------------------------------
> auto client = Http("http://api.vevo.com/mobile/v2/authentication.json");
> client.addHeader("User-Agent", "Android API Connector");
> client.addHeader("Connection", "Keep-Alive");
> client.method = Http.Method.post;
> client.onReceive = (ubyte[] data) { write(cast(char[])data); return
> data.length; };
> client.postData = "p=android&v=1.05";
> client.perform();
>
> //2nd request
> client.url = "http://api.vevo.com/mobile/v1/featured/carousel.json";
> client.method = Http.Method.get;
> client.perform();
> --------------------------------------------
> Maybe something like this. (+points if the code uses existing
> websites)

I'll include that. And I need a "header(key, value)" parameter on Http.(Async)Result as well. That way your example could be written:

auto r = Http.post("http://api.vevo.com/mobile/v2/authentication.json",
                   "p=android&v=1.05")
                   .header("User-Agent", "Android API Connector")
                   .header("Connection", "Keep-Alive"));
write(r.bytes);

> BTW: The curl verbose output is great. I guess it won't
> be activated in phobos by default, but is it possible to activate it
> manually? If so, this very useful feature should be documented.

Yes - verbose should be made in a property by itself.

Thank you for the comments!
/Jonas

Den 19-06-2011 05:16, Jimmy Cao skrev:
> On Sat, Jun 18, 2011 at 4:16 PM, jdrewsen <jdrewsen@nospam.com
> <mailto:jdrewsen@nospam.com>> wrote:
>
>     Den 18-06-2011 22:52, Jimmy Cao skrev:
>
>         Would an SMTP protocol struct be beneficial?
>
>
>     My immediate goal is to provide HTTP support and basic FTP support
>     through libcurl. I believe these are the most important protocols to
>     get in place in order to improve the adoption of D.
>
>     I have currently no plans of adding more protocols to the curl
>     wrapper. Patches are welcome :)
>
>
> Here's a crude implementation for supporting the SMTP protocol:
> https://gist.github.com/1033711
>
> What do you think?

Very nice. A couple of things I believe would help:
1, Get rid of MailMessageData and use curl.onSend() and a delegate that keeps a reference to the message. That way you don't have to use the lower level Curl.set(infile/readfunction) calls as well.

2, It would be nice if the static sendMail(...) function worked like the Http/Ftp counterparts. They return a Result object that you can change before performing the actual task. That way you can easily set timeouts etc. If there shouldn't be support for async smtp then this is probably not important though.

> My biggest concern is whether SMTP protocol support would really be
> necessary within this module.

Personally it is not very high on my list. Initially I would like to have the curl wrapper accepted with only Http and basic Ftp just to reach a milestone. After that other protocols could be added. But thats just how I imagine the process.

/Jonas