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

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

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

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

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

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.

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

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

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

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