Jonas Drewsen wrote: >On 24/08/11 14.58, Johannes Pfau wrote: >> David Nadlinger wrote: >>> Now that Lars Kyllingstad's new and improved std.path has passed the >>> vote – congratulations, Lars! –, and Jose Armando Garcia, the >>> author of the proposed logging module, is currently not available, >>> the etc.curl module by Jonas Drewsen is at the front of the review >>> queue. I have volunteered to run the formal process for inclusion >>> with Phobos. >>> >>> etc.curl provides a high-level interface to client-side networking >>> functionality for the HTTP, FTP and SMTP protocols by wrapping >>> libcurl [1], accessing the C interface of which is already possible >>> using Phobos in the form of etc.c.curl. Prior stages of the work >>> have already been discussed in March [2], May [3], and June [4]. >>> Thanks to everybody who took the time to evaluate the library back >>> then; as far as I am aware, all raised concerns have been addressed >>> since then. >>> >>> Based on the amount of discussion this module already underwent, I >>> hereby propose a two-week review period. Barring any objections, the >>> review starts today (Aug 18) and ends on Aug 31, followed by a one >>> week vote (Sep 1 - Sep 7). >>> >>> Code: >>> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d >>> >>> API documentation: >>> http://freeze.steamwinter.com/D/web/phobos/etc_curl.html >>> >>> A discussion on the wrapper has already been started on >>> digitalmars.D [5]. To keep everything in one place, please post >>> your comments and reviews there. >>> >>> At this point, I would like to invite everyone to spend some time >>> testing the module and reading its documentation and code. It is >>> essential for a working open source code review process that many >>> people participate in it, and regardless of whether you are new to D >>> or a seasoned contributor, your opinion is very valuable. >>> >>> Jonas also asked us to have a look at the list of known issues >>> (which can also be found at the top of the source file), and, if >>> possible, suggest a solution: >>> >>> --- >>> Known issues: >>> * DDoc is not generated where the mixins ByLineAsync, ByLineSync, >>> ByChunkAsync and ByLineSync are used. This seems to be a limitation >>> of ddoc - suggestions on how to circumvent this appreciated. >>> >>> Possible improvements: >>> * Progress may be deprecated in the future. Maybe implement a >>> replacement. >>> * Support typed http headers - (Johannes Pfau) >>> --- >>> >>> To make sure it doesn't get lost, let me also repeat Johannes Pfau's >>> request to add proxy support from another thread ([6]) here. >>> >>> Thanks a lot for your reviewing efforts, >>> David >>> >>> >>> >>> [1] http://curl.haxx.se/libcurl/ >>> [2] >>> http://www.digitalmars.com/d/archives/digitalmars/D/Curl_support_RFC_131753.html >>> [3] >>> http://www.digitalmars.com/d/archives/digitalmars/D/Curl_wrapper_136372.html >>> [4] >>> http://www.digitalmars.com/d/archives/digitalmars/D/Curl_wrapper_round_two_138945.html >>> [5] >>> http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=142661 >>> [6] >>> http://www.digitalmars.com/webnews/newsgroups.php?art_group=digitalmars.D&article_id=142175 >> >> >> +++++++++++++++++++++++++++ Code Review >> +++++++++++++++++++++++++ >> >> Comments and examples: >> /** >> Example: >> ---- >> curl.onReceive = (ubyte[] data) { writeln("Got data", cast(char[]) >> data); return data.length;}; >> ---- >> */ >> Maybe rewrite comments like this: >> /** >> * Example: >> * ---- >> * curl.onReceive = (ubyte[] data) { writeln("Got data", >> * cast(char[]) data); return data.length;}; >> * ---- >> */ >> This way, the examples don't break the indentation. I think this >> "misaligned" example blocks make the source harder to read. >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L152 >> bu --> by; thread --> threads >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L148 >> What does the phobos style guide say for alias names? I'd argue >> outdata/indata is a type (or at least, looks like one), so --> >> OutData/InData ? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L198 >> Do not call this from... --> Warning: Do not call this from... >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L289 >> CurlOption.file --> CurlOption.writeData ? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L326 >> Content of the delegate in the example should be idented. >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L345 >> CurlOption.infile --> CurlOption.readData ? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L362 >> indent >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L414 >> indent >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L420 >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L693 >> dltotal, dlnow, ultotal, ulnow --> dlTotal, dlNow, ulTotal, ulNow ? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L432 >> The C callbacks take char* pointers to the data buffers. As these >> could contain binary data, shouldn't ubyte* be used here? I know that >> the data is later cast to ubyte[], but I'm a little concerned about >> this pointer slicing: str[0..size*nmemb] . This creates a temporary >> char[] which could be invalid. Maybe not a big deal, but I think we >> should avoid that. >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L540 >> It's a little strange that such properties are write-only, but I know >> that's caused by a curl limitation, so nothing we can do about that. >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L576 >> CurlOption.intrface --> CurlOption.interface, likely also wrong in >> etc.c.curl >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L595 >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L584 >> int port --> ushort port? I see no need for negative values? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L645 >> indent >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L662 >> aften --> after >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L739 >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L779 >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L817 >> shouldn't those be documented, so that the user knows which methods >> can be used on Http.Result/AsyncResult? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1176 >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L2464 >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L2899 >> Maybe some mixin documentation should be added manually, something >> like "Mixes in Protocol" >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1431 >> I think I've seen REST APIs which use POST without data or with >> parameters embedded in the URL. Is there no simple way to tell curl >> to _not add_ any data and _not set_ the Content-Type header? But I'm >> not even sure if a POST without a BODY is valid in HTTP. >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1432 >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1500 >> Shouldn't delim be "&"? >> Also in the foreach loop, delim should be ignored for the first pair? >> foreach (i, key; params.byKey()) { >> if(i == 0) >> data ~= key ~ "=" ~ params[key]; >> else >> data ~= delim ~ key ~ "=" ~ params[key]; >> } >> >> Also, is there a reason why the user must escape the parameters >> manually? As the user has no Curl handle, that could be annoying. >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1537 >> const(char)[] k = "Content-Type"; >> res.header(k, contentType); >> --> res.header("Content-Type", contentType); ? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1658 >> if ( _true_ ||!netAllowed) return; ? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1770 >> Add "; charset=utf-8" ? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1789 >> IIRC it's possible to add parameter names to delegate types in D >> to document their use, so >> void delegate(const(char)[],const(char)[]) callback) >> --> void delegate(const(char)[] key,const(char)[] value) callback) >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1798 >> if (header[0..5] == "HTTP/") { >> --> >> if (header.length>= 5&& header[0..5] == "HTTP/") { >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1829 >> if --> of >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L1959 >> add a toString() method for debugging purposes? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L2221 >> document which functions call execute() ? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L2283 >> why private? >> (*v) ~= value; --> (*v) ~= "," ~ value; ? >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L2327 >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L2308 >> Document that this still transfers all data into memory first. >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L2643 >> Document that Result mixes in ProtocolRequestParamsSetters and >> RequestParamsSetters >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L2652 >> Add this bug and all other similar bugs to a ddoc BUG: section in the >> module comment, so this doesn't get lost. >> >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L2932 >> https://github.com/jcd/phobos/blob/curl-wrapper/etc/curl.d#L2943 >> Bounds checking: At least a assert((recipient|sender).length> 0); >> should be added. >> >> (I've skipped most of the async stuff, maybe someone else should >> have a look at that.) >> >> ++++++++++++++++++++++ API docs Review ++++++++++++++++++++++ >> >> First, huge example: >> Split this into many smaller code blocks, add some high-level >> overview, here's some inspiration in pseudo-ddoc, feel free to use >> this: >> >> etc.curl provides wrappers for $(LINK curls) HTTP, FTP and SMTP >> functions. >> >> There are convenience functions which can be used to make simple >> requests with a single line of code: >> ---------------------------------------- >> // Simple HTTP GET >> auto result = Http.get("http://www.google.com";).toString(); >> ---------------------------------------- >> This makes a HTTP GET request to "http://www.google.com";. These >> convenience functions are always _static_ functions of the HTTP / >> FTP / SMTP structs. They return a Result/AsyncResult struct which can >> be used to set options of the request and get the result as a string >> or ubyte[] array. >> ---------------------------------------- >> // Simple HTTP GET with timeout >> auto request = Http.get("http://www.google.com";); >> request.connectTimeout(dur!"seconds"(10)); >> auto result = request.toString(); >> ---------------------------------------- >> As connectTimeout and similar methods return the Result/AsyncResult >> instance, it's possible to chain those calls into a single line: >> ---------------------------------------- >> auto result = >> Http.get("http://www.google.com";).connectTimeout(dur!"seconds"(10)).toString(); >> ---------------------------------------- >> Note that the request to the remote webserver is done when you call >> toString() or similar methods that internally call execute (see >> $(LINK )). If you don't call toString() or a similar method, no >> request is made! >> >> etc.curl additionally provides asynchronous versions of these >> convenience functions: Http.getAsync() is the asynchronous version >> that will spawn a thread in the background and return a >> Http.AsyncResult immediately. You can read data from the result at >> later point in time. This allows you to start processing data before >> all data has been received by using byChunk() or byLine() on the >> Http.AsyncResult instance. >> ---------------------------------------- >> // GET using an asynchronous range >> foreach (line; Http.getAsync("http://www.google.com";).byLine()) >> { >> // Do some expensive processing on the line while more lines are >> // received asynchronously in the background. >> writeln("asyncLine: ", line); >> } >> ---------------------------------------- >> >> If you need more control than the convenience functions provide, >> you'll have to use the full API: >> ---------------------------------------- >> // GET with custom data receivers >> Http http = Http("http://www.google.com";); >> http.onReceiveHeader = >> (const(char)[] key, const(char)[] value) { writeln(key ~ ": " ~ >> value); }; >> http.onReceive = (ubyte[] data) { /+ drop +/ return >> data.length; }; >> http.perform(); >> ---------------------------------------- >> First, we create an instance of the reference-counted Http struct. We >> then set custom delegates which are called whenever the Http instance >> receives a header or a data buffer. In this simple example, we simply >> print the headers and ignore the data. See $(LINK >> onReceiveHeader/onReceive) for more information. >> We finally start the HTTP request by calling perform(). Note that you >> cannot combine the convenience get/post/put/... functions with this >> extended API: Although you could call http.get("http://some.url";) >> instead of perform, this would completely ignore the settings made to >> the Http instance! Remember that get/post/put/etc are _static_ >> methods. They cannot access this instances data. >> >> If you want to make a different request than GET with the extended >> API, you have to set the method property: >> ---------------------------------------- >> // PUT with data senders >> string msg = "Hello world"; >> http.onSend = (void[] data) >> { >> if (msg.empty) >> return 0; >> >> auto m = cast(void[])msg; >> size_t length = m.length; >> >> //We assume msg fits into the data buffer, real code shouldn't >> do //that >> assert(data.length> msg.length); >> data[0..length] = m[0..$]; >> return length; >> }; >> http.method = Http.Method.put; >> http.contentLength = 11; // defaults to chunked transfer if not >> specified >> http.perform(); >> ---------------------------------------- >> If you data doesn't fit into the buffer provided by onSend, onSend >> will be called multiple times. See $(LINK) for more information. >> >> The extended API has also functions to help you track the transfer's >> progress: >> ---------------------------------------- >> // Track progress >> http.method = Http.Method.get; >> http.url = "http://upload.wikimedia.org/wikipedia/commons/"; >> "5/53/Wikipedia-logo-en-big.png"; >> http.onReceive = (ubyte[] data) { return data.length; }; //Discard >> data http.onProgress = (double dltotal, double dlnow, >> double ultotal, double ulnow) { >> writeln("Progress ", dltotal, ", ", dlnow, ", ", ultotal, ", ", >> ulnow); return 0; >> }; >> http.perform(); >> ---------------------------------------- >> >> As a last example, here's how to send an email with etc.curl: >> ---------------------------------------- >> // Send an email with SMTPS >> Smtp smtp = Smtp("smtps://smtp.gmail.com"); >> smtp.setAuthentication("[email protected]", "password"); >> smtp.mailTo = ["<[email protected]>"]; >> smtp.mailFrom = "<[email protected]>"; >> smtp.message = "Example Message"; >> smtp.perform(); >> ---------------------------------------- >> >> Some complete examples like these above could also be added to the >> Http/Ftp/Smtp struct documentation. You could use the examples I used >> in a reply to Walter, if you wanted to. >> >> I'd also say, at least in the examples, use braces on their own >> lines. And I'd insert a few empty lines in certain places, for >> example: ---------------------------------------- >> http.onSend = (void[] data) { >> if (msg.empty) return 0; >> auto m = cast(void[])msg; >> size_t length = m.length; >> data[0..length] = m[0..$]; >> msg.length = 0; >> return length; >> }; >> ---------------------------------------- >> >> doesn't look very good, I'd write it like this: >> ---------------------------------------- >> http.onSend = (void[] data) >> { >> if (msg.empty) >> return 0; >> >> auto m = cast(void[])msg; >> size_t length = m.length; >> >> //We assume msg fits into the data buffer, real code shouldn't >> do //that >> assert(data.length> msg.length); >> data[0..length] = m[0..$]; >> return length; >> }; >> ---------------------------------------- >> >> ++++++++++++++++++++++ Feature request ++++++++++++++++++++++ >> >> Although David already quoted my request in the initial post (thx >> David), I'll repeat it here: >> >> Could you add proxy support to the >> wrapper? All of CURLOPT_PROXY CURLOPT_PROXYPORT and CURLOPT_PROXYTYPE >> should be available. (The 'combined' CURLOPT_PROXY since 7.21.7 is >> too new, that's not even in the current ubuntu) >> > >Thank you. Will chew through it slowly. > >/Jonas > > >
Thanks, I know it's a lot, but most things are small cosmetic changes or typos or similar. -- Johannes Pfau
