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

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

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

>> Hi Jonas,
>>
>> Was reading your implementation but I had to context switch. Only go to line 145 :(. I see that you are refcounting by sharing a uint* but what about all the other private fields? What happens if you pass the Curl object around functions and those values are modified?
>>
>> Thanks,
>> -Jose
>
> The refcounting is actually just needed to keep the "handle" alive under
> construction of the Curl object using "Curl()". I'm using "Curl()" by
> defining opCall on Curl in order not to have a struct constructor with a
> dummy parameter (structs cannot have a default constructor defined).
>
> After that the Curl instance will always be assigned to a member variable of Http/Ftp classes. Instances of Http/Ftp are not to be copied because they are used for RAII.
>
Http/Ftp are structs not classes. Let me try to understand this. You mean to say that the Http and Ftp struct are not to be passed to other functions? Are you expecting the user to do all their IO in one scope? This is unnecessarily limiting.

-Jose

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

jdrewsen wrote:
>Den 21-06-2011 12:49, Johannes Pfau skrev:
>> jdrewsen wrote:
>>> Den 18-06-2011 22:36, jdrewsen skrev:
>>>> Hi,
>>>>
>>>> I've finally got through all the very constructive comments from the last review of the curl wrapper and performed the needed changes.
>>>>
>>>> Here is the github branch: https://github.com/jcd/phobos/tree/curl-wrapper
>>>>
>>>> And the generated docs: http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
>>>
>>> I've made the changes as suggested from your comments and pushed to the github branch above.
>>>
>>> Changes:
>>>
>>> * Change and delete individual headers when using static convenience
>>> methods
>>> * Make keep-alive work when using static convenience methods
>>> * Add as extra modifiable parameters on follow requests
>>> (keep-alive): headers, method, url, postData
>>> * Add verbose property to Protocol
>>> * No dummy bool in constructors
>>>
>>> Comments are welcome
>>>
>>> /Jonas
>>
>> Great, thanks for including those changes!
>> Another small feature request:
>> Could curl_easy_escape and curl_easy_unescape be made available in
>> the wrapper? I don't like the fact that Curl requires a client
>> instance for these functions, but they're quite useful nevertheless.
>
>I'll add this yes. It pretty weird that there is deprecated curl_escape() which does not need a curl instance. That function refers to the newer curl_easy_escape(). They must have had a good reason to do so i guess.
>
>> BTW: http://d-programming-language.org/ has newer css stylesheets for documentation. I think with this new style the documentation looks much better. So it'd be great if you could rebuild the documentation with the new std.ddoc and stylesheets from https://github.com/D-Programming-Language/d-programming-language.org
>
>Done
>
>/Jonas
>
Thanks.

Documentation review:

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

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

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

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

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

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

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

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

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

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

On 22/06/11 06.11, Jose Armando Garcia wrote:
>>> Hi Jonas,
>>>
>>> Was reading your implementation but I had to context switch. Only go
>>> to line 145 :(. I see that you are refcounting by sharing a uint* but
>>> what about all the other private fields? What happens if you pass the
>>> Curl object around functions and those values are modified?
>>>
>>> Thanks,
>>> -Jose
>>
>> The refcounting is actually just needed to keep the "handle" alive under
>> construction of the Curl object using "Curl()". I'm using "Curl()" by
>> defining opCall on Curl in order not to have a struct constructor with a
>> dummy parameter (structs cannot have a default constructor defined).
>>
>> After that the Curl instance will always be assigned to a member variable of
>> Http/Ftp classes. Instances of Http/Ftp are not to be copied because they
>> are used for RAII.
>>
> Http/Ftp are structs not classes. Let me try to understand this. You
> mean to say that the Http and Ftp struct are not to be passed to other
> functions? Are you expecting the user to do all their IO in one scope?
> This is unnecessarily limiting.
>

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

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

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

/Jonas

On 22.06.2011 14:02, Jonas Drewsen wrote:
> On 22/06/11 06.11, Jose Armando Garcia wrote:
>>>> Hi Jonas,
>>>>
>>>> Was reading your implementation but I had to context switch. Only go
>>>> to line 145 :(. I see that you are refcounting by sharing a uint* but
>>>> what about all the other private fields? What happens if you pass the
>>>> Curl object around functions and those values are modified?
>>>>
>>>> Thanks,
>>>> -Jose
>>>
>>> The refcounting is actually just needed to keep the "handle" alive under
>>> construction of the Curl object using "Curl()". I'm using "Curl()" by
>>> defining opCall on Curl in order not to have a struct constructor with a
>>> dummy parameter (structs cannot have a default constructor defined).
>>>
>>> After that the Curl instance will always be assigned to a member variable of
>>> Http/Ftp classes. Instances of Http/Ftp are not to be copied because they
>>> are used for RAII.
>>>
>> Http/Ftp are structs not classes. Let me try to understand this. You
>> mean to say that the Http and Ftp struct are not to be passed to other
>> functions? Are you expecting the user to do all their IO in one scope?
>> This is unnecessarily limiting.
>>
>
> They are structs because they use a Curl instance which in turn uses RAII in order not to leak curl handles. If they were classes it would be easy to leak because you never know when the GC is coming around to call the destructor and release the curl handle.
>
> Your can pass Http/Ftp structs to other function either by givin them a reference to a local instance or you could simply do a:
>
> auto http = new Http("http://google.com");

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

> passItOn(http);
>
> /Jonas


-- 
Dmitry Olshansky

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