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.

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

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

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
>

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

jdrewsen wrote:
>Den 19-06-2011 11:08, Johannes Pfau skrev:
>> jdrewsen wrote:
>>> Hi,
>>>
>>>     I've finally got through all the very constructive comments from
>>> the
>>> last review of the curl wrapper and performed the needed changes.
>>>
>>> Here is the github branch: https://github.com/jcd/phobos/tree/curl-wrapper
>>>
>>> And the generated docs: http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>>
>>> I do have some problems getting ddoc to show the documentation of mixins. So in order to view the doc for byLine/byChunk methods you have to look at the source.
>>
>> That's bad because lots of useful stuff hides in the protocol mixin. The url property for example is essential for keep-alive requests, but it doesn't show up in the documentation :-(
>
>I agree. And also in the ByLineAsync etc. mixins. I would very much like to get a hint on how to do it if anyone knows.
>
>> Also, a keep alive example would be great:
>> --------------------------------------------
>> auto client =
>> Http("http://api.vevo.com/mobile/v2/authentication.json");
>> client.addHeader("User-Agent", "Android API Connector");
>> client.addHeader("Connection", "Keep-Alive"); client.method =
>> Http.Method.post; client.onReceive = (ubyte[] data)
>> { write(cast(char[])data); return data.length; };
>> client.postData = "p=android&v=1.05";
>> client.perform();
>>
>> //2nd request
>> client.url = "http://api.vevo.com/mobile/v1/featured/carousel.json";
>> client.method = Http.Method.get;
>> client.perform();
>> --------------------------------------------
>> Maybe something like this. (+points if the code uses existing
>> websites)
>
>I'll include that.
Feel free to use this example, but please note that the vevo api is not public, so it could break any time.

>And I need a "header(key, value)" parameter on Http.(Async)Result as well. That way your example could be written:
>
>auto r = Http.post("http://api.vevo.com/mobile/v2/authentication.json",
>                    "p=android&v=1.05")
>                    .header("User-Agent", "Android API Connector")
>                    .header("Connection", "Keep-Alive"));
>write(r.bytes);

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

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

>> BTW: The curl verbose output is great. I guess it won't
>> be activated in phobos by default, but is it possible to activate it
>> manually? If so, this very useful feature should be documented.
>
>Yes - verbose should be made in a property by itself.
>
>Thank you for the comments!
>/Jonas
>
You're welcome!

-- 
Johannes Pfau

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

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

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

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