Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23135017:
Edited docs for dart:io HttpRequest (Closed)
DescriptionEdited docs for dart:io HttpRequest
BUG=
R=kathyw@google.com, sgjesse@google.com
Committed: https://code.google.com/p/dart/source/detail?r=26769
Patch Set 1 #
Total comments: 32
Patch Set 2 : incorporated Katwa's review comments. #
Total comments: 16
Patch Set 3 : Changes based on sgjesse and katwa's review comments. #
Total comments: 1
Patch Set 4 : code units -> bytes #Patch Set 5 : code units -> bytes #Messages
Total messages: 8 (0 generated)
Editing doc comments for dart:io HttpRequest. mem
quibbles galore! https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart File sdk/lib/io/http.dart (right): https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:707: * and headers, about the request. I'd move "about the request" up after information, to reduce the amount of commas/interruption. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:710: * uses the HttpRequest object's `method` property to triage requests. should that be [method]? https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:724: * onError: printError); // .listen failed. The "." notation seems weird to me here. How about: .listen -> listen or listen() https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:725: * }, onError: printError); // .then failed. .then -> then or then() https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:731: * For example, here's a skeletal callback function for a request: It'd be nice to see handleRequest specified somewhere (either in this snippet or the one above), so they can make the connection between specifying the callback and implementing it. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:750: * this returns -1. We usually use noun phrases for properties, even for getters. Maybe: The content length... (read-only). OR The content length.... [read only] this returns -> this value is https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:755: * Returns the method, such as 'GET' or 'POST', for the request. The method, ... request (read-only). (or follow whatever convention we agree on) https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:760: * Returns the URI for the request. The URI... (read-only). https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:768: * Returns the request headers. nounify, add "read only" https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:773: * Returns the cookies in the request (from the Cookie headers). nounify, add "read only" Actually, I wonder if "read only" is needed here, since the "from" is a strong hint about that. We should decide on the rules. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:778: * Returns the persistent connection state signaled by the client. nounify https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:783: * Returns the client certificate of the client making the request. nounify, read-only https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:792: * Gets the session for the given request. nounify, read-only https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:802: * Returns the HTTP protocol version used in the request, nounify https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:808: * Gets information about the client connection. nounify, read-only https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:822: HttpResponse get response; nounify, read-only
Adding Søren as reviewer. Thanks. mem https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart File sdk/lib/io/http.dart (right): https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:707: * and headers, about the request. On 2013/08/27 07:18:29, Kathy Walrath wrote: > I'd move "about the request" up after information, to reduce the amount of > commas/interruption. Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:710: * uses the HttpRequest object's `method` property to triage requests. On 2013/08/27 07:18:29, Kathy Walrath wrote: > should that be [method]? Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:724: * onError: printError); // .listen failed. On 2013/08/27 07:18:29, Kathy Walrath wrote: > The "." notation seems weird to me here. How about: > > .listen -> listen or listen() Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:725: * }, onError: printError); // .then failed. On 2013/08/27 07:18:29, Kathy Walrath wrote: > .then -> then or then() Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:731: * For example, here's a skeletal callback function for a request: On 2013/08/27 07:18:29, Kathy Walrath wrote: > It'd be nice to see handleRequest specified somewhere (either in this snippet or > the one above), so they can make the connection between specifying the callback > and implementing it. Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:750: * this returns -1. On 2013/08/27 07:18:29, Kathy Walrath wrote: > We usually use noun phrases for properties, even for getters. Maybe: > > The content length... (read-only). > OR > The content length.... [read only] > > this returns -> this value is Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:755: * Returns the method, such as 'GET' or 'POST', for the request. On 2013/08/27 07:18:29, Kathy Walrath wrote: > The method, ... request (read-only). > > (or follow whatever convention we agree on) Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:760: * Returns the URI for the request. On 2013/08/27 07:18:29, Kathy Walrath wrote: > The URI... (read-only). Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:768: * Returns the request headers. On 2013/08/27 07:18:29, Kathy Walrath wrote: > nounify, add "read only" Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:773: * Returns the cookies in the request (from the Cookie headers). On 2013/08/27 07:18:29, Kathy Walrath wrote: > nounify, add "read only" > > Actually, I wonder if "read only" is needed here, since the "from" is a strong > hint about that. We should decide on the rules. Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:778: * Returns the persistent connection state signaled by the client. On 2013/08/27 07:18:29, Kathy Walrath wrote: > nounify Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:783: * Returns the client certificate of the client making the request. On 2013/08/27 07:18:29, Kathy Walrath wrote: > nounify, read-only Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:792: * Gets the session for the given request. On 2013/08/27 07:18:29, Kathy Walrath wrote: > nounify, read-only Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:802: * Returns the HTTP protocol version used in the request, On 2013/08/27 07:18:29, Kathy Walrath wrote: > nounify Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:808: * Gets information about the client connection. On 2013/08/27 07:18:29, Kathy Walrath wrote: > nounify, read-only Done. https://chromiumcodereview.appspot.com/23135017/diff/1/sdk/lib/io/http.dart#n... sdk/lib/io/http.dart:822: HttpResponse get response; On 2013/08/27 07:18:29, Kathy Walrath wrote: > nounify, read-only Done.
one typo, otherwise lgtm https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dart File sdk/lib/io/http.dart (right): https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:707: * such as the method, URI, and headers, about the request. strike ", about the request"
Thanks for working on the dart:io documentation. I have some comments. We have a package which makes working with the low level dart:io HTTP server subsystem somewhat easier. It is http://pub.dartlang.org/packages/http_server, maybe we should mention it somehow, but maybe we should not refer to packages from the dockumentation for the built-in libraries. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dart File sdk/lib/io/http.dart (right): https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:702: * An [HttpServer] listens for HTTP requests on a specific host and port. This section mentions a callback function. I think it is better to just say that there is a stream of HttpRequests, maybe start with something like this: An [HttpServer] generates a [Stream] of HttpRequest objects. For each HTTP request which comes in a HttpRequest is added to the stream. The HttpRequest object... https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:710: * uses the HttpRequest object's [method] property to triage requests. Maybe change triage to dispatch. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:712: * final HOST = '127.0.0.1'; // localhost. Use InternetAddress.LOOPBACK_IP_V4 instead of '127.0.0.1'. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:726: * }, onError: printError); // then() failed. Future.then does not have an optional named onError argument, but an optional positional argument for the onError. However we encurage users to use catchError instead (the doc for Future.then says: "In most cases, it is more readable to use catchError separately, possibly with a test parameter, instead of handling both value and error in a single then call") So this line should be: }).catchError(printError); https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:736: * print('${req.method}: ${req.uri.path}'); // Print the method and URI. I don't think we should use pint in the examples. How about sending whi in the response - see comment below. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:739: * // buffer contains the content of the request. As this is a stream the buffer might not contain all the request content. This should look something like this instead: // Request data is a stream of bytes. req.fold([], (list, data) => list..add(data)) .then((body) { res.write('Received ${body.length} for request ${req.method}: ${req.uri.path}'); res.close(); }).catchError(printError); or if we want to use listen explicitly: var body = []; req.listen((List<int> buffer) => body.add(buffer), onDone: () { res.write('Received ${body.length} for request ${req.method}: ${req.uri.path}'); res.close(); }, onError(printError)); I have not tried to run this code :-) https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:774: * The cookies in the request (from the Cookie headers), also read-only. also read-only -> (read only) for consistency.
Hi Søren: Thank you for your comments. They were very helpful. I made the changes you suggested (and ran the code with a little client). We are linking to the packages from the dart: libraries and I put it up top since people might want to go to the http_server package first. Is there a class that makes a good starting point in the library? Thanks. mem https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dart File sdk/lib/io/http.dart (right): https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:702: * An [HttpServer] listens for HTTP requests on a specific host and port. On 2013/08/27 09:11:10, Søren Gjesse wrote: > This section mentions a callback function. I think it is better to just say that > there is a stream of HttpRequests, maybe start with something like this: > > An [HttpServer] generates a [Stream] of HttpRequest objects. For each HTTP > request which comes in a HttpRequest is added to the stream. The HttpRequest > object... Done. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:707: * such as the method, URI, and headers, about the request. On 2013/08/27 08:23:11, Kathy Walrath wrote: > strike ", about the request" Done. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:710: * uses the HttpRequest object's [method] property to triage requests. On 2013/08/27 09:11:10, Søren Gjesse wrote: > Maybe change triage to dispatch. Done. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:712: * final HOST = '127.0.0.1'; // localhost. On 2013/08/27 09:11:10, Søren Gjesse wrote: > Use InternetAddress.LOOPBACK_IP_V4 instead of '127.0.0.1'. Done. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:726: * }, onError: printError); // then() failed. On 2013/08/27 09:11:10, Søren Gjesse wrote: > Future.then does not have an optional named onError argument, but an optional > positional argument for the onError. > > However we encurage users to use catchError instead (the doc for Future.then > says: "In most cases, it is more readable to use catchError separately, possibly > with a test parameter, instead of handling both value and error in a single then > call") > > So this line should be: > > }).catchError(printError); Done. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:736: * print('${req.method}: ${req.uri.path}'); // Print the method and URI. On 2013/08/27 09:11:10, Søren Gjesse wrote: > I don't think we should use pint in the examples. How about sending whi in the > response - see comment below. Done. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:739: * // buffer contains the content of the request. On 2013/08/27 09:11:10, Søren Gjesse wrote: > As this is a stream the buffer might not contain all the request content. This > should look something like this instead: > > // Request data is a stream of bytes. > req.fold([], (list, data) => list..add(data)) > .then((body) { > res.write('Received ${body.length} for request ${req.method}: > ${req.uri.path}'); > res.close(); > }).catchError(printError); > > or if we want to use listen explicitly: > > var body = []; > req.listen((List<int> buffer) => body.add(buffer), > onDone: () { > res.write('Received ${body.length} for request ${req.method}: > ${req.uri.path}'); > res.close(); > }, > onError(printError)); > > I have not tried to run this code :-) Done. https://chromiumcodereview.appspot.com/23135017/diff/6001/sdk/lib/io/http.dar... sdk/lib/io/http.dart:774: * The cookies in the request (from the Cookie headers), also read-only. On 2013/08/27 09:11:10, Søren Gjesse wrote: > also read-only -> (read only) > > for consistency. Done.
lgtm https://chromiumcodereview.appspot.com/23135017/diff/12001/sdk/lib/io/http.dart File sdk/lib/io/http.dart (right): https://chromiumcodereview.appspot.com/23135017/diff/12001/sdk/lib/io/http.da... sdk/lib/io/http.dart:713: * as a stream of code units. code units -> bytes
Message was sent while issue was closed.
Committed patchset #5 manually as r26769 (presubmit successful). |
