www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Curl wrapper round two

reply jdrewsen <jdrewsen nospam.com> writes:
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
Jun 18 2011
next sibling parent reply Jimmy Cao <jcao219 gmail.com> writes:
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
Jun 18 2011
parent reply jdrewsen <jdrewsen nospam.com> writes:
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
Jun 18 2011
parent reply Jimmy Cao <jcao219 gmail.com> writes:
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.



 Jun 18 2011



  parent reply jdrewsen <jdrewsen nospam.com>  writes:


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



 Jun 19 2011



  parent reply Jimmy Cao <jcao219 gmail.com>  writes:


On Sun, Jun 19, 2011 at 5:29 AM, jdrewsen <jdrewsen nospam.com> wrote:


 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.



Ah, that makes it much better.




 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.





There should be support for async SMTP.
The problem is this:
SMTP.sendMailAsync(...).connectTimeout(dur!"seconds"(60)).localPort(25).?

byLine, byChunk, etc don't make much sense there.

I think it would be better to get rid of the static sendMail function, and
write a performAsync method.
I'm not sure though.
Something like this:
https://gist.github.com/1034433



 Jun 19 2011



  parent reply jdrewsen <jdrewsen nospam.com>  writes:


Den 19-06-2011 17:57, Jimmy Cao skrev:

 On Sun, Jun 19, 2011 at 5:29 AM, jdrewsen <jdrewsen nospam.com
 <mailto:jdrewsen nospam.com>> wrote:

     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.


 Ah, that makes it much better.

     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.


 There should be support for async SMTP.
 The problem is this:
 SMTP.sendMailAsync(...).connectTimeout(dur!"seconds"(60)).localPort(25).?

 byLine, byChunk, etc don't make much sense there.

 I think it would be better to get rid of the static sendMail function,
 and write a performAsync method.
 I'm not sure though.
 Something like this:
 https://gist.github.com/1034433


Maybe i doesn't make sense to provide the async interface at all? Users 
needing to do it async could just as well create a delegate and call 
spawn themselves.

/Jonas



 Jun 19 2011



  parent  Jimmy Cao <jcao219 gmail.com>  writes:


On Sun, Jun 19, 2011 at 3:03 PM, jdrewsen <jdrewsen nospam.com> wrote:


 Den 19-06-2011 17:57, Jimmy Cao skrev:


 On Sun, Jun 19, 2011 at 5:29 AM, jdrewsen <jdrewsen nospam.com

 <mailto:jdrewsen nospam.com>> wrote:

    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.


 Ah, that makes it much better.

    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.


 There should be support for async SMTP.
 The problem is this:
 SMTP.sendMailAsync(...).**connectTimeout(dur!"seconds"(**
 60)).localPort(25).?

 byLine, byChunk, etc don't make much sense there.

 I think it would be better to get rid of the static sendMail function,
 and write a performAsync method.
 I'm not sure though.
 Something like this:
 https://gist.github.com/**1034433 <https://gist.github.com/1034433>


 Maybe i doesn't make sense to provide the async interface at all? Users
 needing to do it async could just as well create a delegate and call spawn
 themselves.

 /Jonas


Yep.
I think there isn't a need for a static convenience function either.



 Jun 19 2011



prev sibling next sibling parent reply Jimmy Cao <jcao219 gmail.com>  writes:


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







 Jun 18 2011



  parent reply Johannes Pfau <spam example.com>  writes:


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



 Jun 19 2011



  parent reply jdrewsen <jdrewsen nospam.com>  writes:


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.



 Jun 19 2011



 next sibling parent reply Max Klyga <max.klyga gmail.com>  writes:


On 2011-06-19 12:44:57 +0300, jdrewsen said:


 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.


Declare static opCall and then to instatiate:
    auto foo = Bar();
But the user must remember to initialize variables properly.



 Jun 19 2011



 next sibling parent  Jonathan M Davis <jmdavisProg gmx.com>  writes:


On 2011-06-19 04:26, Max Klyga wrote:

 On 2011-06-19 12:44:57 +0300, jdrewsen said:

 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.


 
 Declare static opCall and then to instatiate:
     auto foo = Bar();
 But the user must remember to initialize variables properly.


That's why I _always_ use that syntax when declaring variables, unless I 
really do want init. But while it is a good habit to get into, there's no 
guarantee that the user will have such a habit. But given the inherent issues 
with default constructors and structs, there really isn't much else that we 
can do about it.

- Jonathan M Davis



 Jun 19 2011



prev sibling  parent reply jdrewsen <jdrewsen nospam.com>  writes:


Den 19-06-2011 13:26, Max Klyga skrev:

 On 2011-06-19 12:44:57 +0300, jdrewsen said:


 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.


 Declare static opCall and then to instatiate:
 auto foo = Bar();
 But the user must remember to initialize variables properly.


Thanks.

I guess that I can initialize variables in the opCall and that way the 
user should not have to remember to initialize variables?

/Jonas



 Jun 19 2011



  parent reply David Nadlinger <see klickverbot.at>  writes:


On 6/19/11 7:34 PM, jdrewsen wrote:

 I guess that I can initialize variables in the opCall and that way the
 user should not have to remember to initialize variables?


Yes, you can just do:

---
struct Foo {
   Foo static opCall() {
     Foo result;
     result.<whatever> = <somevalue>;
     return result;
   }
   …
}
---

The thing is that the static opCall is not invoked if the user writes 
»Foo foo;« instead of »auto foo = Foo();«, so you need to carefully 
document that users need to use the opCall syntax.

David



 Jun 19 2011



  parent  jdrewsen <jdrewsen nospam.com>  writes:


Den 19-06-2011 19:42, David Nadlinger skrev:

 On 6/19/11 7:34 PM, jdrewsen wrote:

 I guess that I can initialize variables in the opCall and that way the
 user should not have to remember to initialize variables?


 Yes, you can just do:

 ---
 struct Foo {
 Foo static opCall() {
 Foo result;
 result.<whatever> = <somevalue>;
 return result;
 }
 …
 }
 ---

 The thing is that the static opCall is not invoked if the user writes
 »Foo foo;« instead of »auto foo = Foo();«, so you need to carefully
 document that users need to use the opCall syntax.


This shouldn't be a problem really since the struct is private.

/Jonas



 Jun 19 2011



prev sibling  parent reply Jacob Carlborg <doob me.com>  writes:


On 2011-06-19 11:44, jdrewsen wrote:

 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.


A static opCall function allow this syntax:

auto s = Struct();

-- 
/Jacob Carlborg



 Jun 19 2011



  parent  jdrewsen <jdrewsen nospam.com>  writes:


Den 19-06-2011 14:52, Jacob Carlborg skrev:

 On 2011-06-19 11:44, jdrewsen wrote:

 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.


 A static opCall function allow this syntax:

 auto s = Struct();


Thanks



 Jun 19 2011



prev sibling next sibling parent reply Johannes Pfau <spam example.com>  writes:


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



 Jun 19 2011



  parent reply jdrewsen <jdrewsen nospam.com>  writes:


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



 Jun 19 2011



  parent reply Johannes Pfau <spam example.com>  writes:


jdrewsen wrote:

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.


Feel free to use this example, but please note that the vevo api is not
public, so it could break any time.


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


Looks great, but I guess it won't help for keep-alive sessions? Or is
it possible to reuse the Curl client with the static methods?

I also found another small problem related to keep-alive:
How can I change a header that's been added with addHeader? I have to
reuse the client to use keep-alive, however calling addHeader with the
same key again just appends the header, but it doesn't replace it. An
easy solution is to expose a clearHeader function, but this means if the
user wants to change 1 header all other headers must be set
again. It seems the curl api is too limited to do something more
advanced though. Having a D associative array for the headers might be
possible, but then it has to be converted for curl in every request. 


 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


You're welcome!

-- 
Johannes Pfau



 Jun 19 2011



  parent  jdrewsen <jdrewsen nospam.com>  writes:


Den 19-06-2011 17:43, Johannes Pfau skrev:

 jdrewsen wrote:

 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.


 Feel free to use this example, but please note that the vevo api is not
 public, so it could break any time.


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


 Looks great, but I guess it won't help for keep-alive sessions? Or is
 it possible to reuse the Curl client with the static methods?


Not in the linked version. But is it a nice idea that I'm working on now.


 I also found another small problem related to keep-alive:
 How can I change a header that's been added with addHeader? I have to
 reuse the client to use keep-alive, however calling addHeader with the
 same key again just appends the header, but it doesn't replace it. An
 easy solution is to expose a clearHeader function, but this means if the
 user wants to change 1 header all other headers must be set
 again. It seems the curl api is too limited to do something more
 advanced though. Having a D associative array for the headers might be
 possible, but then it has to be converted for curl in every request.


I'm actually storing a curl_slist to keep the headers. This makes it 
possible to manipulate the headers as you request. I'll fix it so that 
you can change and remove individual headers.



 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


 You're welcome!





 Jun 19 2011



prev sibling next sibling parent reply Jacob Carlborg <doob me.com>  writes:


On 2011-06-18 22:36, 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.

 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


Is the wrapper really supposed to be in the etc package? I thought that 
was just for the bindings?

-- 
/Jacob Carlborg



 Jun 19 2011



  parent  jdrewsen <jdrewsen nospam.com>  writes:


Den 19-06-2011 14:00, Jacob Carlborg skrev:

 On 2011-06-18 22:36, 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.

 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


 Is the wrapper really supposed to be in the etc package? I thought that
 was just for the bindings?


I don't know where best place to put it is. In some way I think modules 
that introduces dependencies (libcurl in this case) is best handled by 
the hopefully upcoming dget/build2/??? functionality and thereby keeping 
phobos free of licensing issues.

Then there could be some official wrappers that are "blessed" meaning 
that the phobos community ensures that the quality is as good as phobos 
itself. I think the curl wrapper would fit in there nicely.

If this can be agreed upon then the module should probably be moved out 
of the etc package and become a root module. The etc.c.curl should 
probably be moved out of phobos at the same time.

/Jonas



 Jun 19 2011



prev sibling next sibling parent reply David Nadlinger <see klickverbot.at>  writes:


Does curl_global_init really have to be called for every thread? If not, 
invoke it in a shared static constructor.

David


On 6/18/11 10:36 PM, 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.

 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





 Jun 19 2011



  parent  jdrewsen <jdrewsen nospam.com>  writes:


Den 19-06-2011 14:03, David Nadlinger skrev:

 Does curl_global_init really have to be called for every thread? If not,
 invoke it in a shared static constructor.


I should indeed only be called once for all threads. Thanks.


 David


 On 6/18/11 10:36 PM, 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.

 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







 Jun 19 2011



prev sibling next sibling parent reply jdrewsen <jdrewsen nospam.com>  writes:


Den 18-06-2011 22:36, jdrewsen skrev:

 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've made the changes as suggested from your comments and pushed to the 
github branch above.

Changes:

* Change and delete individual headers when using static convenience methods
* Make keep-alive work when using static convenience methods
* Add as extra modifiable parameters on follow requests (keep-alive): 
headers, method, url, postData
* Add verbose property to Protocol
* No dummy bool in constructors

Comments are welcome

/Jonas



 Jun 20 2011



 next sibling parent reply Jose Armando Garcia <jsancio gmail.com>  writes:


On Mon, Jun 20, 2011 at 8:33 PM, jdrewsen <jdrewsen nospam.com> wrote:

 Den 18-06-2011 22:36, jdrewsen skrev:

 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've made the changes as suggested from your comments and pushed to the
 github branch above.

 Changes:

 * Change and delete individual headers when using static convenience methods
 * Make keep-alive work when using static convenience methods
 * Add as extra modifiable parameters on follow requests (keep-alive):
 headers, method, url, postData
 * Add verbose property to Protocol
 * No dummy bool in constructors

 Comments are welcome

 /Jonas


Hi Jonas,

Was reading your implementation but I had to context switch. Only go
to line 145 :(. I see that you are refcounting by sharing a uint* but
what about all the other private fields? What happens if you pass the
Curl object around functions and those values are modified?

Thanks,
-Jose



 Jun 20 2011



  parent reply jdrewsen <jdrewsen nospam.com>  writes:


Den 21-06-2011 02:52, Jose Armando Garcia skrev:

 On Mon, Jun 20, 2011 at 8:33 PM, jdrewsen<jdrewsen nospam.com>  wrote:

 Den 18-06-2011 22:36, jdrewsen skrev:

 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've made the changes as suggested from your comments and pushed to the
 github branch above.

 Changes:

 * Change and delete individual headers when using static convenience methods
 * Make keep-alive work when using static convenience methods
 * Add as extra modifiable parameters on follow requests (keep-alive):
 headers, method, url, postData
 * Add verbose property to Protocol
 * No dummy bool in constructors

 Comments are welcome

 /Jonas


 Hi Jonas,

 Was reading your implementation but I had to context switch. Only go
 to line 145 :(. I see that you are refcounting by sharing a uint* but
 what about all the other private fields? What happens if you pass the
 Curl object around functions and those values are modified?

 Thanks,
 -Jose


The refcounting is actually just needed to keep the "handle" alive under 
construction of the Curl object using "Curl()". I'm using "Curl()" by 
defining opCall on Curl in order not to have a struct constructor with a 
dummy parameter (structs cannot have a default constructor defined).

After that the Curl instance will always be assigned to a member 
variable of Http/Ftp classes. Instances of Http/Ftp are not to be copied 
because they are used for RAII.

I hope this makes sense.

Maybe I should look for another solution for this since it might be too 
difficult to figure out for the uninitiated.

/Jonas



 Jun 21 2011



  parent reply Jose Armando Garcia <jsancio gmail.com>  writes:


 Hi Jonas,

 Was reading your implementation but I had to context switch. Only go
 to line 145 :(. I see that you are refcounting by sharing a uint* but
 what about all the other private fields? What happens if you pass the
 Curl object around functions and those values are modified?

 Thanks,
 -Jose


 The refcounting is actually just needed to keep the "handle" alive under
 construction of the Curl object using "Curl()". I'm using "Curl()" by
 defining opCall on Curl in order not to have a struct constructor with a
 dummy parameter (structs cannot have a default constructor defined).

 After that the Curl instance will always be assigned to a member variable of
 Http/Ftp classes. Instances of Http/Ftp are not to be copied because they
 are used for RAII.


Http/Ftp are structs not classes. Let me try to understand this. You
mean to say that the Http and Ftp struct are not to be passed to other
functions? Are you expecting the user to do all their IO in one scope?
This is unnecessarily limiting.

-Jose



 Jun 21 2011



  parent reply Jonas Drewsen <jdrewsen nospam.com>  writes:


On 22/06/11 06.11, Jose Armando Garcia wrote:

 Hi Jonas,

 Was reading your implementation but I had to context switch. Only go
 to line 145 :(. I see that you are refcounting by sharing a uint* but
 what about all the other private fields? What happens if you pass the
 Curl object around functions and those values are modified?

 Thanks,
 -Jose


 The refcounting is actually just needed to keep the "handle" alive under
 construction of the Curl object using "Curl()". I'm using "Curl()" by
 defining opCall on Curl in order not to have a struct constructor with a
 dummy parameter (structs cannot have a default constructor defined).

 After that the Curl instance will always be assigned to a member variable of
 Http/Ftp classes. Instances of Http/Ftp are not to be copied because they
 are used for RAII.


 Http/Ftp are structs not classes. Let me try to understand this. You
 mean to say that the Http and Ftp struct are not to be passed to other
 functions? Are you expecting the user to do all their IO in one scope?
 This is unnecessarily limiting.


They are structs because they use a Curl instance which in turn uses 
RAII in order not to leak curl handles. If they were classes it would be 
easy to leak because you never know when the GC is coming around to call 
the destructor and release the curl handle.

Your can pass Http/Ftp structs to other function either by givin them a 
reference to a local instance or you could simply do a:

auto http = new Http("http://google.com");
passItOn(http);

/Jonas



 Jun 22 2011



 next sibling parent reply Dmitry Olshansky <dmitry.olsh gmail.com>  writes:


On 22.06.2011 14:02, Jonas Drewsen wrote:

 On 22/06/11 06.11, Jose Armando Garcia wrote:

 Hi Jonas,

 Was reading your implementation but I had to context switch. Only go
 to line 145 :(. I see that you are refcounting by sharing a uint* but
 what about all the other private fields? What happens if you pass the
 Curl object around functions and those values are modified?

 Thanks,
 -Jose


 The refcounting is actually just needed to keep the "handle" alive 
 under
 construction of the Curl object using "Curl()". I'm using "Curl()" by
 defining opCall on Curl in order not to have a struct constructor 
 with a
 dummy parameter (structs cannot have a default constructor defined).

 After that the Curl instance will always be assigned to a member 
 variable of
 Http/Ftp classes. Instances of Http/Ftp are not to be copied because 
 they
 are used for RAII.


 Http/Ftp are structs not classes. Let me try to understand this. You
 mean to say that the Http and Ftp struct are not to be passed to other
 functions? Are you expecting the user to do all their IO in one scope?
 This is unnecessarily limiting.


 They are structs because they use a Curl instance which in turn uses 
 RAII in order not to leak curl handles. If they were classes it would 
 be easy to leak because you never know when the GC is coming around to 
 call the destructor and release the curl handle.

 Your can pass Http/Ftp structs to other function either by givin them 
 a reference to a local instance or you could simply do a:

 auto http = new Http("http://google.com");


I suspect that http is on _heap_ just like class, and it's destructor is 
never called ATM. Even when that's fixed it's still non-deterministic.
Maybe refcounting? e.g. std.typecons.RefCounted


 passItOn(http);

 /Jonas



-- 
Dmitry Olshansky



 Jun 22 2011



  parent  jdrewsen <jdrewsen nospam.com>  writes:


Den 22-06-2011 12:08, Dmitry Olshansky skrev:

 On 22.06.2011 14:02, Jonas Drewsen wrote:

 On 22/06/11 06.11, Jose Armando Garcia wrote:

 Hi Jonas,

 Was reading your implementation but I had to context switch. Only go
 to line 145 :(. I see that you are refcounting by sharing a uint* but
 what about all the other private fields? What happens if you pass the
 Curl object around functions and those values are modified?

 Thanks,
 -Jose


 The refcounting is actually just needed to keep the "handle" alive
 under
 construction of the Curl object using "Curl()". I'm using "Curl()" by
 defining opCall on Curl in order not to have a struct constructor
 with a
 dummy parameter (structs cannot have a default constructor defined).

 After that the Curl instance will always be assigned to a member
 variable of
 Http/Ftp classes. Instances of Http/Ftp are not to be copied because
 they
 are used for RAII.


 Http/Ftp are structs not classes. Let me try to understand this. You
 mean to say that the Http and Ftp struct are not to be passed to other
 functions? Are you expecting the user to do all their IO in one scope?
 This is unnecessarily limiting.


 They are structs because they use a Curl instance which in turn uses
 RAII in order not to leak curl handles. If they were classes it would
 be easy to leak because you never know when the GC is coming around to
 call the destructor and release the curl handle.

 Your can pass Http/Ftp structs to other function either by givin them
 a reference to a local instance or you could simply do a:

 auto http = new Http("http://google.com");


 I suspect that http is on _heap_ just like class, and it's destructor is
 never called ATM. Even when that's fixed it's still non-deterministic.
 Maybe refcounting? e.g. std.typecons.RefCounted


 passItOn(http);

 /Jonas




As I replied a minute ago to Jose I'll change it to match the 
std.stdio.File behavior.

/Jonas



 Jun 22 2011



prev sibling  parent reply Jose Armando Garcia <jsancio gmail.com>  writes:


Em 22/06/2011, =C3=A0s 07:02, Jonas Drewsen <jdrewsen nospam.com> =
escreveu:


 On 22/06/11 06.11, Jose Armando Garcia wrote:

 Hi Jonas,

 Was reading your implementation but I had to context switch. Only =20=









 go
 to line 145 :(. I see that you are refcounting by sharing a uint* =20=









 but
 what about all the other private fields? What happens if you pass =20=









 the
 Curl object around functions and those values are modified?

 Thanks,
 -Jose


 The refcounting is actually just needed to keep the "handle" alive =20=







 under
 construction of the Curl object using "Curl()". I'm using "Curl()" =20=







 by
 defining opCall on Curl in order not to have a struct constructor =20=







 with a
 dummy parameter (structs cannot have a default constructor defined).

 After that the Curl instance will always be assigned to a member =20
 variable of
 Http/Ftp classes. Instances of Http/Ftp are not to be copied =20
 because they
 are used for RAII.


 Http/Ftp are structs not classes. Let me try to understand this. You
 mean to say that the Http and Ftp struct are not to be passed to =20
 other
 functions? Are you expecting the user to do all their IO in one =20
 scope?
 This is unnecessarily limiting.


 They are structs because they use a Curl instance which in turn uses =20=



 RAII in order not to leak curl handles. If they were classes it =20
 would be easy to leak because you never know when the GC is coming =20
 around to call the destructor and release the curl handle.

 Your can pass Http/Ftp structs to other function either by givin =20
 them a reference to a local instance or you could simply do a:

 auto http =3D new Http("http://google.com");
 passItOn(http);

 /Jonas


Passing refs around doesn't work if the function wants to store it on =20=

the heap for later.

Have you taken a look at how std.stdio.File and =20
std.typecons.RefCounted do reference counting?=



 Jun 22 2011



  parent  jdrewsen <jdrewsen nospam.com>  writes:


Den 22-06-2011 17:34, Jose Armando Garcia skrev:

 Em 22/06/2011, às 07:02, Jonas Drewsen <jdrewsen nospam.com> escreveu:


 On 22/06/11 06.11, Jose Armando Garcia wrote:

 Hi Jonas,

 Was reading your implementation but I had to context switch. Only go
 to line 145 :(. I see that you are refcounting by sharing a uint* but
 what about all the other private fields? What happens if you pass the
 Curl object around functions and those values are modified?

 Thanks,
 -Jose


 The refcounting is actually just needed to keep the "handle" alive
 under
 construction of the Curl object using "Curl()". I'm using "Curl()" by
 defining opCall on Curl in order not to have a struct constructor
 with a
 dummy parameter (structs cannot have a default constructor defined).

 After that the Curl instance will always be assigned to a member
 variable of
 Http/Ftp classes. Instances of Http/Ftp are not to be copied because
 they
 are used for RAII.


 Http/Ftp are structs not classes. Let me try to understand this. You
 mean to say that the Http and Ftp struct are not to be passed to other
 functions? Are you expecting the user to do all their IO in one scope?
 This is unnecessarily limiting.


 They are structs because they use a Curl instance which in turn uses
 RAII in order not to leak curl handles. If they were classes it would
 be easy to leak because you never know when the GC is coming around to
 call the destructor and release the curl handle.

 Your can pass Http/Ftp structs to other function either by givin them
 a reference to a local instance or you could simply do a:

 auto http = new Http("http://google.com");
 passItOn(http);

 /Jonas


 Passing refs around doesn't work if the function wants to store it on
 the heap for later.

 Have you taken a look at how std.stdio.File and std.typecons.RefCounted
 do reference counting?


I think I'll change it to match std.stdio.File in the name of consistency.

Thanks
/Jonas



 Jun 22 2011



prev sibling next sibling parent reply Johannes Pfau <spam example.com>  writes:


jdrewsen wrote:

Den 18-06-2011 22:36, jdrewsen skrev:

 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've made the changes as suggested from your comments and pushed to
the github branch above.

Changes:

* Change and delete individual headers when using static convenience
methods
* Make keep-alive work when using static convenience methods
* Add as extra modifiable parameters on follow requests (keep-alive): 
headers, method, url, postData
* Add verbose property to Protocol
* No dummy bool in constructors

Comments are welcome

/Jonas


Great, thanks for including those changes!
Another small feature request:
Could curl_easy_escape and curl_easy_unescape be made available in the
wrapper? I don't like the fact that Curl requires a client instance for
these functions, but they're quite useful nevertheless.

BTW: http://d-programming-language.org/ has newer css stylesheets for
documentation. I think with this new style the documentation looks much
better. So it'd be great if you could rebuild the documentation with
the new std.ddoc and stylesheets from
https://github.com/D-Programming-Language/d-programming-language.org

-- 
Johannes Pfau



 Jun 21 2011



  parent reply jdrewsen <jdrewsen nospam.com>  writes:


Den 21-06-2011 12:49, Johannes Pfau skrev:

 jdrewsen wrote:

 Den 18-06-2011 22:36, jdrewsen skrev:

 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've made the changes as suggested from your comments and pushed to
 the github branch above.

 Changes:

 * Change and delete individual headers when using static convenience
 methods
 * Make keep-alive work when using static convenience methods
 * Add as extra modifiable parameters on follow requests (keep-alive):
 headers, method, url, postData
 * Add verbose property to Protocol
 * No dummy bool in constructors

 Comments are welcome

 /Jonas


 Great, thanks for including those changes!
 Another small feature request:
 Could curl_easy_escape and curl_easy_unescape be made available in the
 wrapper? I don't like the fact that Curl requires a client instance for
 these functions, but they're quite useful nevertheless.


I'll add this yes. It pretty weird that there is deprecated 
curl_escape() which does not need a curl instance. That function refers 
to the newer curl_easy_escape(). They must have had a good reason to do 
so i guess.


 BTW: http://d-programming-language.org/ has newer css stylesheets for
 documentation. I think with this new style the documentation looks much
 better. So it'd be great if you could rebuild the documentation with
 the new std.ddoc and stylesheets from
 https://github.com/D-Programming-Language/d-programming-language.org


Done

/Jonas



 Jun 21 2011



  parent reply Johannes Pfau <spam example.com>  writes:


jdrewsen wrote:

Den 21-06-2011 12:49, Johannes Pfau skrev:

 jdrewsen wrote:

 Den 18-06-2011 22:36, jdrewsen skrev:

 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've made the changes as suggested from your comments and pushed to
 the github branch above.

 Changes:

 * Change and delete individual headers when using static convenience
 methods
 * Make keep-alive work when using static convenience methods
 * Add as extra modifiable parameters on follow requests
 (keep-alive): headers, method, url, postData
 * Add verbose property to Protocol
 * No dummy bool in constructors

 Comments are welcome

 /Jonas


 Great, thanks for including those changes!
 Another small feature request:
 Could curl_easy_escape and curl_easy_unescape be made available in
 the wrapper? I don't like the fact that Curl requires a client
 instance for these functions, but they're quite useful nevertheless.


I'll add this yes. It pretty weird that there is deprecated 
curl_escape() which does not need a curl instance. That function
refers to the newer curl_easy_escape(). They must have had a good
reason to do so i guess.


 BTW: http://d-programming-language.org/ has newer css stylesheets for
 documentation. I think with this new style the documentation looks
 much better. So it'd be great if you could rebuild the documentation
 with the new std.ddoc and stylesheets from
 https://github.com/D-Programming-Language/d-programming-language.org


Done

/Jonas


Thanks.

Documentation review:

First example:
http.perform; -- http.perform();
Property syntax should only be used for properties.

Protocol.isStopped:
True if the instance is stopped an[d] invalid.

Protocol.dataTimeout:
Connection settings Set timeout for activity on connection
'Connections settings' should be a normal comment in the source code?

Protocol.url
Network settings The URL to specify the location of the resource
'Network settings' should be a normal comment in the source code?

Protocol.setAuthentication:
Set the use[r]name, pas[s]word and optionally domain for authentication
purposes.

Protocol.onSend:
See onSend">Curl.onSend
Is "> really expected?

alias TimeCond:
Maybe add to the docs that TimeCond is an alias for CurlTimeCond?
none ifmodsince ifunmodsince lastmod
last ---> TimeCond.{none,ifmodsince,ifunmodsince,lastmod} ?

Some one-sentence comments have a '.' at the end, others
don't. This should maybe changed to be consistent.

Wonder if there's a good free way to spellcheck a website, that could
be quite useful for the phobos reviews.
-- 
Johannes Pfau



 Jun 22 2011



  parent  Jacob Carlborg <doob me.com>  writes:


On 2011-06-22 11:08, Johannes Pfau wrote:

 Wonder if there's a good free way to spellcheck a website, that could
 be quite useful for the phobos reviews.


Doesn't most text editors have a spell check feature? Even vim has it.

-- 
/Jacob Carlborg



 Jun 22 2011



prev sibling  parent reply Johannes Pfau <spam example.com>  writes:


jdrewsen wrote:

Den 18-06-2011 22:36, jdrewsen skrev:

 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've made the changes as suggested from your comments and pushed to
the github branch above.

Changes:

* Change and delete individual headers when using static convenience
methods
* Make keep-alive work when using static convenience methods
* Add as extra modifiable parameters on follow requests (keep-alive): 
headers, method, url, postData
* Add verbose property to Protocol
* No dummy bool in constructors

Comments are welcome

/Jonas


Oh, and btw are you reading the phobos-dev mailing list? Seems like
there are no code reviews planned right now, so if you think etc.curl
was ready for review, you could propose it now?

-- 
Johannes Pfau



 Jun 21 2011



  parent reply jdrewsen <jdrewsen nospam.com>  writes:


Den 21-06-2011 12:55, Johannes Pfau skrev:

 jdrewsen wrote:

 Den 18-06-2011 22:36, jdrewsen skrev:

 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've made the changes as suggested from your comments and pushed to
 the github branch above.

 Changes:

 * Change and delete individual headers when using static convenience
 methods
 * Make keep-alive work when using static convenience methods
 * Add as extra modifiable parameters on follow requests (keep-alive):
 headers, method, url, postData
 * Add verbose property to Protocol
 * No dummy bool in constructors

 Comments are welcome

 /Jonas


 Oh, and btw are you reading the phobos-dev mailing list? Seems like
 there are no code reviews planned right now, so if you think etc.curl
 was ready for review, you could propose it now?


It don't know anything about a phobos-dev mailing list. The only reviews 
I've seen so far is on this D newsgroup. How do I get on that mailing list?

I'll think It'll soon be ready for an official code review though.

/Jonas



 Jun 21 2011



  parent reply Johannes Pfau <spam example.com>  writes:


jdrewsen wrote:

Den 21-06-2011 12:55, Johannes Pfau skrev:

 jdrewsen wrote:

 Den 18-06-2011 22:36, jdrewsen skrev:

 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've made the changes as suggested from your comments and pushed to
 the github branch above.

 Changes:

 * Change and delete individual headers when using static convenience
 methods
 * Make keep-alive work when using static convenience methods
 * Add as extra modifiable parameters on follow requests
 (keep-alive): headers, method, url, postData
 * Add verbose property to Protocol
 * No dummy bool in constructors

 Comments are welcome

 /Jonas


 Oh, and btw are you reading the phobos-dev mailing list? Seems like
 there are no code reviews planned right now, so if you think etc.curl
 was ready for review, you could propose it now?


It don't know anything about a phobos-dev mailing list. The only
reviews I've seen so far is on this D newsgroup. How do I get on that
mailing list?


Here's the web interface:
http://lists.puremagic.com/mailman/listinfo (the 'phobos' list)
Reviews are always done on the newsgroup, but some phobos related
discussion is also happening on the mailing list.


I'll think It'll soon be ready for an official code review though.

/Jonas



-- 
Johannes Pfau



 Jun 22 2011



  parent  jdrewsen <jdrewsen nospam.com>  writes:


Den 22-06-2011 10:25, Johannes Pfau skrev:

 jdrewsen wrote:

 Den 21-06-2011 12:55, Johannes Pfau skrev:

 jdrewsen wrote:

 Den 18-06-2011 22:36, jdrewsen skrev:

 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've made the changes as suggested from your comments and pushed to
 the github branch above.

 Changes:

 * Change and delete individual headers when using static convenience
 methods
 * Make keep-alive work when using static convenience methods
 * Add as extra modifiable parameters on follow requests
 (keep-alive): headers, method, url, postData
 * Add verbose property to Protocol
 * No dummy bool in constructors

 Comments are welcome

 /Jonas


 Oh, and btw are you reading the phobos-dev mailing list? Seems like
 there are no code reviews planned right now, so if you think etc.curl
 was ready for review, you could propose it now?


 It don't know anything about a phobos-dev mailing list. The only
 reviews I've seen so far is on this D newsgroup. How do I get on that
 mailing list?


 Here's the web interface:
 http://lists.puremagic.com/mailman/listinfo (the 'phobos' list)
 Reviews are always done on the newsgroup, but some phobos related
 discussion is also happening on the mailing list.


Thanks!


 I'll think It'll soon be ready for an official code review though.

 /Jonas







 Jun 22 2011



prev sibling  parent  Jacob Carlborg <doob me.com>  writes:


On 2011-06-18 22:36, 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.

 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


A couple of comments:

* The documentation for the callback functions are referring to Curl 
which is not documented.

* Why are you using "typeof" in the PUT example?

* The PUT example seems unnecessary complicated. Why can't the "onSend" 
callback simply return the data instead, am I missing something obvious?

-- 
/Jacob Carlborg



 Jun 22 2011