digitalmars.D - Curl wrapper review
- Jonas Drewsen <jdrewsen nospam.com> Nov 15 2011
- Bernard Helyer <b.helyer gmail.com> Nov 15 2011
- Jonas Drewsen <jdrewsen nospam.com> Nov 16 2011
- David Nadlinger <see klickverbot.at> Nov 15 2011
- Jonas Drewsen <jdrewsen nospam.com> Nov 16 2011
- Sam Hu <samhu.samhu gmail.com> Nov 15 2011
- Johannes Pfau <spam example.com> Nov 16 2011
- Jonas Drewsen <jdrewsen nospam.com> Nov 17 2011
- Jacob Carlborg <doob me.com> Nov 17 2011
- Jonas Drewsen <jdrewsen nospam.com> Nov 17 2011
- Jonas Drewsen <jdrewsen nospam.com> Nov 18 2011
- Johannes Pfau <spam example.com> Nov 17 2011
- Jonas Drewsen <jdrewsen nospam.com> Nov 18 2011
- Johannes Pfau <spam example.com> Nov 18 2011
- Jonas Drewsen <jdrewsen nospam.com> Nov 19 2011
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
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
Jonas Drewsen <jdrewsen nospam.com> 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 privateIf 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
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
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
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
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
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
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
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
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
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
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
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
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