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:
--001485f9a6ee7ecbfe04a602b0bc
Content-Type: text/plain; charset=ISO-8859-1

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

--001485f9a6ee7ecbfe04a602b0bc Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Would an SMTP protocol struct be beneficial?<div><br></div><div>This looks = great, thanks for you work.<br><div><br><div class=3D"gmail_quote">On Sat, = Jun 18, 2011 at 3:36 PM, jdrewsen <span dir=3D"ltr">&lt;<a href=3D"mailto:j= drewsen nospam.com" target=3D"_blank">jdrewsen nospam.com</a>&gt;</span> wr= ote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex">Hi,<br> <br> =A0 I&#39;ve finally got through all the very constructive comments from t= he last review of the curl wrapper and performed the needed changes.<br> <br> Here is the github branch:<br> <a href=3D"https://github.com/jcd/phobos/tree/curl-wrapper" target=3D"_blan= k">https://github.com/jcd/phobos/<u></u>tree/curl-wrapper</a><br> <br> And the generated docs:<br> <a href=3D"http://freeze.steamwinter.com/D/web/phobos/etc_curl.html" target= =3D"_blank">http://freeze.steamwinter.com/<u></u>D/web/phobos/etc_curl.html= </a><br> <br> I do have some problems getting ddoc to show the documentation of mixins. S= o in order to view the doc for byLine/byChunk methods you have to look at t= he source.<br> <br> Anyway...this is what I&#39;ve been up to:<br> <br> New features:<br> <br> * Full support for async/sync by line/chunk<br> * FTP support extended from only allowing download of a file sync into full= async/sync by line/chunk support<br> * Allow providing parameters such as credentials/timeouts when using the co= nvenience statis methods.<br> <br> Changes caused by last review:<br> <br> * rethink byLine/... to not return string in order to prevent allocations. = they should return char[]/ubyte[]<br> * 80 chars<br> * Http.Result not HttpResult<br> * gramma for http.postData<br> * len -&gt; length<br> * perform http request -&gt; perform a http ...<br> * authMethod to property<br> * curltimecond alias into module<br> * followlocation -&gt; maxredirs<br> * http not class anymore but struct<br> * timecondition use std.datetime<br> * timeouts use core.duration<br> * Spelling &quot;callbacks is not supported&quot;<br> * refer to HTTP RFC describing the methods<br> * login/password example<br> * chuncked -&gt; chunked<br> * max redirs; use uint.max and not -1<br> * isRunning returining short<br> * 4 chars tabs in examples.<br> * no space in examples.<br> * Send/recv use special structs in order not to mess with other communicati= ons<br> <br> Comments are welcome.<br> <br> /Jonas<br> <br> </blockquote></div><br></div> </div> --001485f9a6ee7ecbfe04a602b0bc--
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 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 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
prev sibling next sibling parent reply Jimmy Cao <jcao219 gmail.com> writes:
--0016363b7fec3f681e04a602ee5f
Content-Type: text/plain; charset=ISO-8859-1

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


--0016363b7fec3f681e04a602ee5f Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Also, why the=A0<span class=3D"Apple-style-span" style=3D"font-family: mono= space; white-space: pre-wrap; font-size: medium; ">bool dummy argument in t= he Curl struct constructor?</span><br><br><div class=3D"gmail_quote">On Sat= , Jun 18, 2011 at 3:52 PM, Jimmy Cao <span dir=3D"ltr">&lt;<a href=3D"mailt= o:jcao219 gmail.com">jcao219 gmail.com</a>&gt;</span> wrote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex;"><div><div><div></div><div class=3D"h5"><br>= <div><br><div class=3D"gmail_quote">On Sat, Jun 18, 2011 at 3:36 PM, jdrews= en <span dir=3D"ltr">&lt;<a href=3D"mailto:jdrewsen nospam.com" target=3D"_= blank">jdrewsen nospam.com</a>&gt;</span> wrote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex">Hi,<br> <br> =A0 I&#39;ve finally got through all the very constructive comments from t= he last review of the curl wrapper and performed the needed changes.<br> <br> Here is the github branch:<br> <a href=3D"https://github.com/jcd/phobos/tree/curl-wrapper" target=3D"_blan= k">https://github.com/jcd/phobos/<u></u>tree/curl-wrapper</a><br> <br> And the generated docs:<br> <a href=3D"http://freeze.steamwinter.com/D/web/phobos/etc_curl.html" target= =3D"_blank">http://freeze.steamwinter.com/<u></u>D/web/phobos/etc_curl.html= </a><br> <br> I do have some problems getting ddoc to show the documentation of mixins. S= o in order to view the doc for byLine/byChunk methods you have to look at t= he source.<br> <br> Anyway...this is what I&#39;ve been up to:<br> <br> New features:<br> <br> * Full support for async/sync by line/chunk<br> * FTP support extended from only allowing download of a file sync into full= async/sync by line/chunk support<br> * Allow providing parameters such as credentials/timeouts when using the co= nvenience statis methods.<br> <br> Changes caused by last review:<br> <br> * rethink byLine/... to not return string in order to prevent allocations. = they should return char[]/ubyte[]<br> * 80 chars<br> * Http.Result not HttpResult<br> * gramma for http.postData<br> * len -&gt; length<br> * perform http request -&gt; perform a http ...<br> * authMethod to property<br> * curltimecond alias into module<br> * followlocation -&gt; maxredirs<br> * http not class anymore but struct<br> * timecondition use std.datetime<br> * timeouts use core.duration<br> * Spelling &quot;callbacks is not supported&quot;<br> * refer to HTTP RFC describing the methods<br> * login/password example<br> * chuncked -&gt; chunked<br> * max redirs; use uint.max and not -1<br> * isRunning returining short<br> * 4 chars tabs in examples.<br> * no space in examples.<br> * Send/recv use special structs in order not to mess with other communicati= ons<br> <br> Comments are welcome.<br> <br> /Jonas<br> <br> </blockquote></div><br></div> </div></div></div> </blockquote></div><br> --0016363b7fec3f681e04a602ee5f--
Jun 18 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
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 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 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 Jimmy Cao <jcao219 gmail.com> writes:
--90e6ba4fc3ae9df70304a6080dd8
Content-Type: text/plain; charset=ISO-8859-1

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

https://gist.github.com/1033711 What do you think? My biggest concern is whether SMTP protocol support would really be necessary within this module. --90e6ba4fc3ae9df70304a6080dd8 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On Sat, Jun 18, 2011 at 4:16 PM, jdrewsen <span dir=3D"ltr">&lt;<a href=3D"= mailto:jdrewsen nospam.com">jdrewsen nospam.com</a>&gt;</span> wrote:<br><d= iv class=3D"gmail_quote"><blockquote class=3D"gmail_quote" style=3D"margin:= 0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> Den 18-06-2011 22:52, Jimmy Cao skrev:<div class=3D"im"><br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"> Would an SMTP protocol struct be beneficial?<br> </blockquote> <br></div> 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 i= n order to improve the adoption of D.<br> <br> I have currently no plans of adding more protocols to the curl wrapper. Pat= ches are welcome :)<br> <br></blockquote><div><br></div><div>Here&#39;s a crude implementation for = supporting the SMTP protocol:</div><div><a href=3D"https://gist.github.com/= 1033711">https://gist.github.com/1033711</a>=A0</div><div><br></div><div>Wh= at do you think?</div> <div>My biggest concern is whether SMTP protocol support would really be ne= cessary within this module.</div></div> --90e6ba4fc3ae9df70304a6080dd8--
Jun 18 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 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.

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


Jun 19 2011
prev sibling next sibling parent 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
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 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.

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

-- Johannes Pfau
Jun 19 2011
prev sibling next sibling parent Jimmy Cao <jcao219 gmail.com> writes:
--0016e64f80006e4a2004a612ad5e
Content-Type: text/plain; charset=ISO-8859-1

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 --0016e64f80006e4a2004a612ad5e Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On Sun, Jun 19, 2011 at 5:29 AM, jdrewsen <span dir=3D"ltr">&lt;<a href=3D"= mailto:jdrewsen nospam.com">jdrewsen nospam.com</a>&gt;</span> wrote:<div c= lass=3D"gmail_quote"><blockquote class=3D"gmail_quote" style=3D"margin:0 0 = 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> Very nice. A couple of things I believe would help:<br> 1, Get rid of MailMessageData and use curl.onSend() and a delegate that kee= ps a reference to the message. That way you don&#39;t have to use the lower= level Curl.set(infile/readfunction) calls as well.</blockquote><div><br> </div><div>Ah, that makes it much better.=A0</div><div><br></div><div>=A0</= div><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-lef= t:1px #ccc solid;padding-left:1ex;"> 2, It would be nice if the static sendMail(...) function worked like the Ht= tp/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 t= here shouldn&#39;t be support for async smtp then this is probably not impo= rtant though.=A0</blockquote> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex;">=A0</blockquote></div><br><div>There should= be support for async SMTP.</div><div>The problem is this:</div><div>SMTP.s= endMailAsync(...).connectTimeout(dur!&quot;seconds&quot;(60)).localPort(25)= .?</div> <div><br></div><div>byLine, byChunk, etc don&#39;t make much sense there.</= div><div><br></div><div>I think it would be better to get rid of the static= sendMail function, and write a performAsync method.</div><div>I&#39;m not = sure though.</div> <div>Something like this:</div><div><a href=3D"https://gist.github.com/1034= 433">https://gist.github.com/1034433</a></div> --0016e64f80006e4a2004a612ad5e--
Jun 19 2011
prev sibling next sibling parent Jimmy Cao <jcao219 gmail.com> writes:
--0016e64f8000d8e8bb04a6177271
Content-Type: text/plain; charset=ISO-8859-1

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. --0016e64f8000d8e8bb04a6177271 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable On Sun, Jun 19, 2011 at 3:03 PM, jdrewsen <span dir=3D"ltr">&lt;<a href=3D"= mailto:jdrewsen nospam.com">jdrewsen nospam.com</a>&gt;</span> wrote:<br><d= iv class=3D"gmail_quote"><blockquote class=3D"gmail_quote" style=3D"margin:= 0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> Den 19-06-2011 17:57, Jimmy Cao skrev:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"> On Sun, Jun 19, 2011 at 5:29 AM, jdrewsen &lt;<a href=3D"mailto:jdrewsen no= spam.com" target=3D"_blank">jdrewsen nospam.com</a><div class=3D"im"><br> &lt;mailto:<a href=3D"mailto:jdrewsen nospam.com" target=3D"_blank">jdrewse= n nospam.com</a>&gt;&gt; wrote:<br> <br></div><div class=3D"im"> =A0 =A0Very nice. A couple of things I believe would help:<br> =A0 =A01, Get rid of MailMessageData and use curl.onSend() and a delegate<= br> =A0 =A0that keeps a reference to the message. That way you don&#39;t have = to<br> =A0 =A0use the lower level Curl.set(infile/readfunction) calls as well.<br=

<br></div><div class=3D"im"> Ah, that makes it much better.<br> <br></div><div class=3D"im"> =A0 =A02, It would be nice if the static sendMail(...) function worked lik= e<br> =A0 =A0the Http/Ftp counterparts. They return a Result object that you can= <br> =A0 =A0change before performing the actual task. That way you can easily<b= r> =A0 =A0set timeouts etc. If there shouldn&#39;t be support for async smtp = then<br> =A0 =A0this is probably not important though.<br> <br> <br></div><div class=3D"im"> There should be support for async SMTP.<br> The problem is this:<br> SMTP.sendMailAsync(...).<u></u>connectTimeout(dur!&quot;seconds&quot;(<u></= u>60)).localPort(25).?<br> <br> byLine, byChunk, etc don&#39;t make much sense there.<br> <br> I think it would be better to get rid of the static sendMail function,<br> and write a performAsync method.<br> I&#39;m not sure though.<br> Something like this:<br> <a href=3D"https://gist.github.com/1034433" target=3D"_blank">https://gist.= github.com/<u></u>1034433</a><br> </div></blockquote> <br> Maybe i doesn&#39;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.<br> <br> /Jonas<br> </blockquote></div><br><div>Yep.</div><div>I think there isn&#39;t a need f= or a static convenience function either.</div> --0016e64f8000d8e8bb04a6177271--
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 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 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.

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.

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.

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

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 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 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 next sibling 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 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 next sibling parent 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.

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
prev sibling parent 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.

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
prev sibling next sibling parent 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
prev sibling next sibling parent 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
prev sibling next sibling parent 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
prev sibling next sibling parent 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
prev sibling next sibling parent 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

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