www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Curl wrapper review

reply Jonas Drewsen <jdrewsen nospam.com> writes:
Hi,

    After all the comments from last review I've refactored the curl 
wrapper and it is ready for a new review.

David Nadlinger was handling the last review so I guess it would make 
sense if he run this one as well if he wants to.

Code:
https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

Docs:
http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

Regards,
Jonas
Nov 15 2011
next sibling parent reply Bernard Helyer <b.helyer gmail.com> writes:
I'll just post my thoughts here while they're fresh. It looks good. The 
documentation is what I'd expect from a Phobos module, as is the naming 
convention.

auto _basicFtp(T)(const(char)[] url, const(void)[] sendData, Ftp client)

If you don't want people using it, shouldn't it be marked private instead 
of using the underscore for obscurity?

private struct Pool(DATA) 
{
private:

You've marked private things as 'private foo;' everywhere else in the 
module, what's with the switch in styles for this struct? Also, as the 
whole struct is module private I'm not sure of the utility of marking 
members private. I guess it's a form of documentation.

But really, I'm grasping at straws. Even if the above were to remain, I 
would love to see this in Phobos yesterday. :)

-Bernard.
Nov 15 2011
parent Jonas Drewsen <jdrewsen nospam.com> writes:
Den 16-11-2011 00:08, Bernard Helyer skrev:
 I'll just post my thoughts here while they're fresh. It looks good. The
 documentation is what I'd expect from a Phobos module, as is the naming
 convention.

 auto _basicFtp(T)(const(char)[] url, const(void)[] sendData, Ftp client)

You're right. Should be private
 If you don't want people using it, shouldn't it be marked private instead
 of using the underscore for obscurity?

 private struct Pool(DATA)
 {
 private:

 You've marked private things as 'private foo;' everywhere else in the
 module, what's with the switch in styles for this struct? Also, as the
 whole struct is module private I'm not sure of the utility of marking
 members private. I guess it's a form of documentation.

Regarding the switch in I just think that I'd been doing lots of c++ coding that day. Anyway I think it is ok to use that style for small classes/structs that can fit on a single screen even though I haven't done that consistently either in this module. I'll change it to match the rest of the module. Regarding the second question: I just think it is good style to mark up the private parts. And if someone copy/pastes it as a public struct it'll work as intended.
 But really, I'm grasping at straws. Even if the above were to remain, I
 would love to see this in Phobos yesterday. :)

 -Bernard.

Thanks for the comments. /Jonas
Nov 16 2011
prev sibling next sibling parent reply David Nadlinger <see klickverbot.at> writes:
Hi Jonas,

great to see that the code is ready for a second round – the sooner we 
get something like this into Phobos, the better.

However, currently I can barely manage to spare some time for D related 
projects, and there are other things I'd really like to finish soon 
(tracking yet another exception-related problem that breaks part of my 
GSoC code on Linux x86_64, updating LDC to DMD 2.056/LLVM 3.0 and 
officially completing its move to GitHub), so it would be great if 
somebody else could handle the review.

David


On 11/15/11 9:21 PM, Jonas Drewsen wrote:
 Hi,

 After all the comments from last review I've refactored the curl wrapper
 and it is ready for a new review.

 David Nadlinger was handling the last review so I guess it would make
 sense if he run this one as well if he wants to.

 Code:
 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

 Docs:
 http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

 Regards,
 Jonas

Nov 15 2011
parent Jonas Drewsen <jdrewsen nospam.com> writes:
That's ok. Anyone else up for handling the review?

/Jonas


Den 16-11-2011 00:36, David Nadlinger skrev:
 Hi Jonas,

 great to see that the code is ready for a second round – the sooner we
 get something like this into Phobos, the better.

 However, currently I can barely manage to spare some time for D related
 projects, and there are other things I'd really like to finish soon
 (tracking yet another exception-related problem that breaks part of my
 GSoC code on Linux x86_64, updating LDC to DMD 2.056/LLVM 3.0 and
 officially completing its move to GitHub), so it would be great if
 somebody else could handle the review.

 David


 On 11/15/11 9:21 PM, Jonas Drewsen wrote:
 Hi,

 After all the comments from last review I've refactored the curl wrapper
 and it is ready for a new review.

 David Nadlinger was handling the last review so I guess it would make
 sense if he run this one as well if he wants to.

 Code:
 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

 Docs:
 http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

 Regards,
 Jonas


Nov 16 2011
prev sibling next sibling parent Sam Hu <samhu.samhu gmail.com> writes:
Jonas Drewsen Wrote:

 Hi,
 
     After all the comments from last review I've refactored the curl 
 wrapper and it is ready for a new review.
 
 David Nadlinger was handling the last review so I guess it would make 
 sense if he run this one as well if he wants to.
 
 Code:
 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d
 
 Docs:
 http://freeze.steamwinter.com/D/web/phobos/etc_curl.html
 
 Regards,
 Jonas

May I ask where should I get curl.lib except using dmc to compile the curl source and build a lib file?Don't understand why *.lib in D/dmc is not compatable with the one os have.
Nov 15 2011
prev sibling next sibling parent reply Johannes Pfau <spam example.com> writes:
Jonas Drewsen wrote:
Hi,

    After all the comments from last review I've refactored the curl 
wrapper and it is ready for a new review.

David Nadlinger was handling the last review so I guess it would make 
sense if he run this one as well if he wants to.

Code:
https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

Docs:
http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

Regards,
Jonas

Looks great so far! Some minor nitpicks: * Examples: "new Http()" is used 4 times in the examples, but as Http is a struct, I see no reason to create it using new? * Move ---------------------------------------------- // Get using an line range and proxy settings auto client = new Http(); client.proxy = "1.2.3.4"; foreach (line; byLine("d-p-l.org", client)) writeln(line); ---------------------------------------------- from the first to the second example. (The sentence "For more control than the high level functions provide, use the low level API:" sounds like everything shown up to that point was the high level api, so that one example should be moved below that sentence) * smtp.perform; --> smtp.perform(); * Please set default timeouts for the simple API. Nothing is more annoying than a application locking up because of internet loss.. * Speaking of timeouts, I'd like a special TimeoutException, as it's easy to recover from these errors. * At least the high-level API should really be safe (or trusted). Adding those qualifiers to the low level API would also be good, but that can always be done after merging into phobos. * Some functions could be nothrow? property StatusLine statusLine() for example, maybe some more. -- Johannes Pfau
Nov 16 2011
parent reply Jonas Drewsen <jdrewsen nospam.com> writes:
On 16/11/11 20.21, Johannes Pfau wrote:
 Jonas Drewsen wrote:
 Hi,

     After all the comments from last review I've refactored the curl
 wrapper and it is ready for a new review.

 David Nadlinger was handling the last review so I guess it would make
 sense if he run this one as well if he wants to.

 Code:
 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

 Docs:
 http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

 Regards,
 Jonas

Looks great so far! Some minor nitpicks: * Examples: "new Http()" is used 4 times in the examples, but as Http is a struct, I see no reason to create it using new?

new Http() calls opCall() where a bunch of stuff is initialized.
 * Move
    ----------------------------------------------
    // Get using an line range and proxy settings
    auto client = new Http();
    client.proxy = "1.2.3.4";
    foreach (line; byLine("d-p-l.org", client))
        writeln(line);
    ----------------------------------------------
    from the first to the second example. (The sentence "For more control
    than the high level functions provide, use the low level API:" sounds
    like everything shown up to that point was the high level api, so that
    one example should be moved below that sentence)

It is stille the high level API. It is just an example of how to set special options using the high level API. The low level API is the one that requires you to register callbacks and convert data yourself etc.
 * smtp.perform; -->  smtp.perform();

ok
 * Please set default timeouts for the simple API. Nothing is more
    annoying than a application locking up because of internet loss..

ok
 * Speaking of timeouts, I'd like a special TimeoutException, as it's
    easy to recover from these errors.

ok
 * At least the high-level API should really be  safe (or  trusted).
    Adding those qualifiers to the low level API would also be good, but
    that can always be done after merging into phobos.

I looked at using safe and trusted as well but skipped it - maybe someone can tell me why this is a good thing to do. I do understand the value of tagging functions with safe, trusted and system. What I fail to see is why the hole concept is not degraded when safe is allowed to call trusted. A programmer would look at the safe function definition and think it is safe and it doesn't do weird casts etc. But this is misleading when trusted functions are called from inside the safe function. Shouldn't trusted taint safe functions and make the compiler complain when trusted is called from within a safe function. I _have_ read the specs. and do know that this is how it is supposed to work. I just need some reasoning behind it.
 * Some functions could be nothrow?  property StatusLine statusLine()
    for example, maybe some more.

I went through all functions to check for nothrow but since RefCounted in almost all functions and methods on the RefCounted is not nothrow I cannot mark anything as nothrow myself. Thanks /Jonas
Nov 17 2011
next sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2011-11-17 10:16, Jonas Drewsen wrote:
 On 16/11/11 20.21, Johannes Pfau wrote:
 Jonas Drewsen wrote:
 Hi,

 After all the comments from last review I've refactored the curl
 wrapper and it is ready for a new review.

 David Nadlinger was handling the last review so I guess it would make
 sense if he run this one as well if he wants to.

 Code:
 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

 Docs:
 http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

 Regards,
 Jonas

Looks great so far! Some minor nitpicks: * Examples: "new Http()" is used 4 times in the examples, but as Http is a struct, I see no reason to create it using new?

new Http() calls opCall() where a bunch of stuff is initialized.

As far as I know "new Http()" does not call opCall, at least not the static one. Use this to call opCall: "auto http = Http()"; -- /Jacob Carlborg
Nov 17 2011
parent Jonas Drewsen <jdrewsen nospam.com> writes:
On 17/11/11 11.54, Jacob Carlborg wrote:
 On 2011-11-17 10:16, Jonas Drewsen wrote:
 On 16/11/11 20.21, Johannes Pfau wrote:
 Jonas Drewsen wrote:
 Hi,

 After all the comments from last review I've refactored the curl
 wrapper and it is ready for a new review.

 David Nadlinger was handling the last review so I guess it would make
 sense if he run this one as well if he wants to.

 Code:
 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

 Docs:
 http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

 Regards,
 Jonas

Looks great so far! Some minor nitpicks: * Examples: "new Http()" is used 4 times in the examples, but as Http is a struct, I see no reason to create it using new?

new Http() calls opCall() where a bunch of stuff is initialized.

As far as I know "new Http()" does not call opCall, at least not the static one. Use this to call opCall: "auto http = Http()";

You're right of course. /Jonas
Nov 17 2011
prev sibling parent Jonas Drewsen <jdrewsen nospam.com> writes:
On 17/11/11 12.42, Johannes Pfau wrote:
 Jonas Drewsen wrote:
 On 16/11/11 20.21, Johannes Pfau wrote:
 Jonas Drewsen wrote:
 Hi,

      After all the comments from last review I've refactored the curl
 wrapper and it is ready for a new review.

 David Nadlinger was handling the last review so I guess it would
 make sense if he run this one as well if he wants to.

 Code:
 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

 Docs:
 http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

 Regards,
 Jonas

Looks great so far! Some minor nitpicks: * Examples: "new Http()" is used 4 times in the examples, but as Http is a struct, I see no reason to create it using new?

new Http() calls opCall() where a bunch of stuff is initialized.

I'm not complaining about the initialization, but about using 'new'. ______________________ auto client = Http(); ---------------------- will call the opCall (and you also have this example in the docs), but it'll allocate client on the stack, where new will use the GC to allocate it on the heap.
 * Move
     ----------------------------------------------
     // Get using an line range and proxy settings
     auto client = new Http();
     client.proxy = "1.2.3.4";
     foreach (line; byLine("d-p-l.org", client))
         writeln(line);
     ----------------------------------------------
     from the first to the second example. (The sentence "For more
 control than the high level functions provide, use the low level
 API:" sounds like everything shown up to that point was the high
 level api, so that one example should be moved below that sentence)

It is stille the high level API. It is just an example of how to set special options using the high level API. The low level API is the one that requires you to register callbacks and convert data yourself etc.

OK I'd agree with that, but the cheat sheet clearly says: * High level: download,upload,get,put,byLine,byChunk,byLineAsync,byChunkAsync * Low level: Http, Ftp, Smtp so the docs are a little inconsistent in that regard.
 * smtp.perform; -->   smtp.perform();

ok
 * Please set default timeouts for the simple API. Nothing is more
     annoying than a application locking up because of internet loss..

ok
 * Speaking of timeouts, I'd like a special TimeoutException, as it's
     easy to recover from these errors.

ok
 * At least the high-level API should really be  safe (or  trusted).
     Adding those qualifiers to the low level API would also be good,
 but that can always be done after merging into phobos.

I looked at using safe and trusted as well but skipped it - maybe someone can tell me why this is a good thing to do. I do understand the value of tagging functions with safe, trusted and system. What I fail to see is why the hole concept is not degraded when safe is allowed to call trusted. A programmer would look at the safe function definition and think it is safe and it doesn't do weird casts etc. But this is misleading when trusted functions are called from inside the safe function. Shouldn't trusted taint safe functions and make the compiler complain when trusted is called from within a safe function. I _have_ read the specs. and do know that this is how it is supposed to work. I just need some reasoning behind it.

safe tells the compiler: This function only uses the safeD subset, complain if I do unsafe stuff. trusted means: This function can do anything, but the programmer guarantees that it's safe to call this function from safeD. This means it's not possible to pass values to the function which can undermine the safeD guarantees: //NOT trusted! (This method breaks if a invalid pointer is passed) void noTrust(int* ptr) { *ptr = 0; } int value; trusted void trustMe(int* ptr) { //Do something to verify input if(ptr !=&value) return; *ptr = 0; } So there should be few trusted code and it should be well tested, but it is necessary to have some sort of 'bridge' between safe and unsafe code. To put it short, trusted allows you to write wrapper functions for system code.

Thats how I understand it as well. My issue is that safe attribute on a function really degrades to trusted if the function calls a trusted function. This means that a programmer that sees a safe attribute on a function really can't rely on the safe properties of calling it. But this is only a documentation issue I guess. Anyway I'll put safe and trusted attr. on the appropriate functions. /Jonas
Nov 18 2011
prev sibling next sibling parent Johannes Pfau <spam example.com> writes:
Jonas Drewsen wrote:
On 16/11/11 20.21, Johannes Pfau wrote:
 Jonas Drewsen wrote:
 Hi,

     After all the comments from last review I've refactored the curl
 wrapper and it is ready for a new review.

 David Nadlinger was handling the last review so I guess it would
 make sense if he run this one as well if he wants to.

 Code:
 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

 Docs:
 http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

 Regards,
 Jonas

Looks great so far! Some minor nitpicks: * Examples: "new Http()" is used 4 times in the examples, but as Http is a struct, I see no reason to create it using new?

new Http() calls opCall() where a bunch of stuff is initialized.

I'm not complaining about the initialization, but about using 'new'. ______________________ auto client = Http(); ---------------------- will call the opCall (and you also have this example in the docs), but it'll allocate client on the stack, where new will use the GC to allocate it on the heap.
 * Move
    ----------------------------------------------
    // Get using an line range and proxy settings
    auto client = new Http();
    client.proxy = "1.2.3.4";
    foreach (line; byLine("d-p-l.org", client))
        writeln(line);
    ----------------------------------------------
    from the first to the second example. (The sentence "For more
 control than the high level functions provide, use the low level
 API:" sounds like everything shown up to that point was the high
 level api, so that one example should be moved below that sentence)

It is stille the high level API. It is just an example of how to set special options using the high level API. The low level API is the one that requires you to register callbacks and convert data yourself etc.

OK I'd agree with that, but the cheat sheet clearly says: * High level: download,upload,get,put,byLine,byChunk,byLineAsync,byChunkAsync * Low level: Http, Ftp, Smtp so the docs are a little inconsistent in that regard.
 * smtp.perform; -->  smtp.perform();

ok
 * Please set default timeouts for the simple API. Nothing is more
    annoying than a application locking up because of internet loss..

ok
 * Speaking of timeouts, I'd like a special TimeoutException, as it's
    easy to recover from these errors.

ok
 * At least the high-level API should really be  safe (or  trusted).
    Adding those qualifiers to the low level API would also be good,
 but that can always be done after merging into phobos.

I looked at using safe and trusted as well but skipped it - maybe someone can tell me why this is a good thing to do. I do understand the value of tagging functions with safe, trusted and system. What I fail to see is why the hole concept is not degraded when safe is allowed to call trusted. A programmer would look at the safe function definition and think it is safe and it doesn't do weird casts etc. But this is misleading when trusted functions are called from inside the safe function. Shouldn't trusted taint safe functions and make the compiler complain when trusted is called from within a safe function. I _have_ read the specs. and do know that this is how it is supposed to work. I just need some reasoning behind it.

safe tells the compiler: This function only uses the safeD subset, complain if I do unsafe stuff. trusted means: This function can do anything, but the programmer guarantees that it's safe to call this function from safeD. This means it's not possible to pass values to the function which can undermine the safeD guarantees: //NOT trusted! (This method breaks if a invalid pointer is passed) void noTrust(int* ptr) { *ptr = 0; } int value; trusted void trustMe(int* ptr) { //Do something to verify input if(ptr != &value) return; *ptr = 0; } So there should be few trusted code and it should be well tested, but it is necessary to have some sort of 'bridge' between safe and unsafe code. To put it short, trusted allows you to write wrapper functions for system code. -- Johannes Pfau
Nov 17 2011
prev sibling next sibling parent Jonas Drewsen <jdrewsen nospam.com> writes:
On 15/11/11 21.21, Jonas Drewsen wrote:
 Hi,

 After all the comments from last review I've refactored the curl wrapper
 and it is ready for a new review.

 David Nadlinger was handling the last review so I guess it would make
 sense if he run this one as well if he wants to.

 Code:
 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

 Docs:
 http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

 Regards,
 Jonas

So it is not possible for David to be review manager atm. Anyone else wanna step up? In case you're new to this: We are using the boost review process and the followin link explains what a review manager should do: http://www.boost.org/community/reviews.html#Review_Manager /Jonas
Nov 18 2011
prev sibling parent reply Johannes Pfau <spam example.com> writes:
Jonas Drewsen wrote:
Hi,

    After all the comments from last review I've refactored the curl 
wrapper and it is ready for a new review.

David Nadlinger was handling the last review so I guess it would make 
sense if he run this one as well if he wants to.

Code:
https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

Docs:
http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

Regards,
Jonas

I found a small bug: void setCookieJar(const(char)[] path) { p.curl.set(CurlOption.cookiefile, path); p.curl.set(CurlOption.cookiejar, path); } It's common to pass an empty string for the cookiefile option, this way curl handles cookies in memory. But the empty string shouldn't be passed to the cookiejar option, as this causes this warning in verbose mode: "* WARNING: failed to save cookies in " -- Johannes Pfau
Nov 18 2011
parent Jonas Drewsen <jdrewsen nospam.com> writes:
Den 18-11-2011 21:53, Johannes Pfau skrev:
 Jonas Drewsen wrote:
 Hi,

     After all the comments from last review I've refactored the curl
 wrapper and it is ready for a new review.

 David Nadlinger was handling the last review so I guess it would make
 sense if he run this one as well if he wants to.

 Code:
 https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d

 Docs:
 http://freeze.steamwinter.com/D/web/phobos/etc_curl.html

 Regards,
 Jonas

I found a small bug: void setCookieJar(const(char)[] path) { p.curl.set(CurlOption.cookiefile, path); p.curl.set(CurlOption.cookiejar, path); } It's common to pass an empty string for the cookiefile option, this way curl handles cookies in memory. But the empty string shouldn't be passed to the cookiejar option, as this causes this warning in verbose mode: "* WARNING: failed to save cookies in "

Ok.. will fix. Thanks Jonas
Nov 19 2011