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


This is the one I dream to have in D.Thanks.
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 reply 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
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 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