Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
299443013:
Add initial version of http_base package (Closed)
|
Created:
6 years, 7 months ago by kustermann Modified:
6 years, 6 months ago CC:
reviews_dartlang.org, kevmoo Visibility:
Public. |
DescriptionAdd initial version of http_base package
R=sgjesse@google.com
Committed: https://code.google.com/p/dart/source/detail?r=36622
Patch Set 1 #Patch Set 2 : #
Total comments: 43
Patch Set 3 : Addressed comments #
Messages
Total messages: 9 (0 generated)
lgtm https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... File dart/pkg/http_base/lib/http_base.dart (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:101: * Function for performing an http request. http -> HTTP Here is says 'an HTTP', and other places 'a HTTP'. It should be consistent. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:119: * Function for handling an http request. http -> HTTP https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:123: * ignored by an Http Server. Http -> HTTP https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:126: * another [HttpRequestHandler] and augment the headers with a 'Set-Cookie' with -> with e.g. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/pubsp... File dart/pkg/http_base/pubspec.yaml (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/pubsp... dart/pkg/http_base/pubspec.yaml:6: A library with common interfaces for http request objects, http response http -> HTTP
In the future, could you update your base url to https://dart.googlecode.com/svn/branches/bleeding_edge/dart This is common across the platform and makes patching in a CL possible https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/CHANG... File dart/pkg/http_base/CHANGELOG.md (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/CHANG... dart/pkg/http_base/CHANGELOG.md:3: * Add a README file. Unneeded in the first commit. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/READM... File dart/pkg/http_base/README.md (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/READM... dart/pkg/http_base/README.md:2: handler interface as well as the corresponding request and response classes. http -> HTTP Use the actual class names here, denoted as [Request], [Response], etc. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... File dart/pkg/http_base/lib/http_base.dart (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:9: /** All packages are moving towards triple-slash "///" doc comments. See tools/line_doc_comments.dart https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:19: * Returns true if a header field of the specified [name] exist. `true` https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:31: * For the 'Cookie' and 'Set-Cookie' headers the comma-separated list We should not specify an undefined behavior for a base interface. Specifying either `null` or throw InvalidSomething... is better. I'd likely lean towards throw. Meaning, it always throws, even if the cookie header is not set w/ a clear message telling the caller to use getMultiple https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:69: Stream<List<int>> get body; Use a method here: read() In many cases, the body can only be accessed once. Documentation should highlight this. See https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/shelf/lib/src... https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:85: String get reasonPhrase; Can we skip this? It's not supported in HTTP 2.0 - http://http2.github.io/http2-spec/#HttpHeaders We could create an class in a helper package (http_parser?) that has exposes the Map<int, String> for each StatusCode https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:95: Stream<List<int>> get body; read() here, too. Same as above. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:116: typedef Future<Response> HttpClient(Request request); We must not export two identical typedefs. Not worth the confusion. Happy to have a conversation about a name that works well on both sides. For now, I'd argue that we get rid of HttpRequestHandler https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:116: typedef Future<Response> HttpClient(Request request); There is already an HttpClient in dart:io. We DON'T want to collide w/ that type.
https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... File dart/pkg/http_base/lib/http_base.dart (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:116: typedef Future<Response> HttpClient(Request request); Requester? https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:132: typedef Future<Response> HttpRequestHandler(Request request); Eliminate this typedef. We only have confusion if we have two identical types.
Thanks for your comments :) The comments are, as expected, minor details and I'll address them and submit this CL very soon. > In the future, could you update your base url to > https://dart.googlecode.com/svn/branches/bleeding_edge/dart > This is common across the platform and makes patching in a CL possible No, I cannot. I do have very good reasons why I do this. But feel free to adopt my model, I can tell you that it results in less issues :) Patching is not a problem: Just pick the correct X in "patch -pX < patch.diff" and you'll be fine.
https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... File dart/pkg/http_base/lib/http_base.dart (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:9: /** On 2014/05/23 21:22:24, kevmoo wrote: > All packages are moving towards triple-slash "///" doc comments. > > See tools/line_doc_comments.dart In my opinion the "///" doc comments are harder to read, and does not stand out as much in an editor. But maybe I am just old fashioned. Nothing in our style guide to suggest one over the other. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:31: * For the 'Cookie' and 'Set-Cookie' headers the comma-separated list On 2014/05/23 21:22:24, kevmoo wrote: > We should not specify an undefined behavior for a base interface. > > Specifying either `null` or throw InvalidSomething... is better. > > I'd likely lean towards throw. Meaning, it always throws, even if the cookie > header is not set w/ a clear message telling the caller to use getMultiple If we should specify something here lets go for throwing. Something like "For header field-names which does not allow combining multiple values with comma using this index operator will throw `IllegalArgument`. This is currently the case for header field-names 'Cookie' and 'Set-Cookie'. Use the `getMultiple` method to iterate the header values for these". https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:80: int get statusCode; If we remove reasonPhrase lets change this to just 'status'. In SPDY and HTTP/2.0 it is the header-field name ':status'. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:85: String get reasonPhrase; On 2014/05/23 21:22:24, kevmoo wrote: > Can we skip this? > > It's not supported in HTTP 2.0 - http://http2.github.io/http2-spec/#HttpHeaders > > We could create an class in a helper package (http_parser?) that has exposes the > Map<int, String> for each StatusCode I am OK with that - I does not add much value. The HTTP status code enumeration could be in this package - it is pretty basic. We already have it once in dart:io. Lets think about it before we add to much here for now. For HTTP 1.1 and SPDY a server should add a reason phrase. It can be empty, but can cause issues if it is. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:116: typedef Future<Response> HttpClient(Request request); On 2014/05/23 21:22:24, kevmoo wrote: > We must not export two identical typedefs. Not worth the confusion. > > Happy to have a conversation about a name that works well on both sides. > > For now, I'd argue that we get rid of HttpRequestHandler I agree that this can be confusing. However the "client" side will be sending the request over some network transport having some server actually handle the request. A "client" might "decorate" the request object to handle e.g. authentication before sending the request. The "server" side is supposed to actually perform the processing of the request. HttpClient could just be Client. However a client is not really handling the request - some server is.
https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/CHANG... File dart/pkg/http_base/CHANGELOG.md (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/CHANG... dart/pkg/http_base/CHANGELOG.md:3: * Add a README file. On 2014/05/23 21:22:24, kevmoo wrote: > Unneeded in the first commit. Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/READM... File dart/pkg/http_base/README.md (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/READM... dart/pkg/http_base/README.md:2: handler interface as well as the corresponding request and response classes. On 2014/05/23 21:22:24, kevmoo wrote: > http -> HTTP > > Use the actual class names here, denoted as [Request], [Response], etc. Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... File dart/pkg/http_base/lib/http_base.dart (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:9: /** On 2014/05/26 07:12:46, Søren Gjesse wrote: > On 2014/05/23 21:22:24, kevmoo wrote: > > All packages are moving towards triple-slash "///" doc comments. > > > > See tools/line_doc_comments.dart > > In my opinion the "///" doc comments are harder to read, and does not stand out > as much in an editor. But maybe I am just old fashioned. Nothing in our style > guide to suggest one over the other. If you say "All packages are moving towards triple-slash". May I ask who started this and for what reason? And why do we say to our users in the style guide we've got 2 different equivalent ways of commenting but we pick the triple-slash way? https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:19: * Returns true if a header field of the specified [name] exist. On 2014/05/23 21:22:24, kevmoo wrote: > `true` Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:31: * For the 'Cookie' and 'Set-Cookie' headers the comma-separated list On 2014/05/26 07:12:46, Søren Gjesse wrote: > On 2014/05/23 21:22:24, kevmoo wrote: > > We should not specify an undefined behavior for a base interface. > > > > Specifying either `null` or throw InvalidSomething... is better. > > > > I'd likely lean towards throw. Meaning, it always throws, even if the cookie > > header is not set w/ a clear message telling the caller to use getMultiple > > If we should specify something here lets go for throwing. Something like "For > header field-names which does not allow combining multiple values with comma > using this index operator will throw `IllegalArgument`. This is currently the > case for header field-names 'Cookie' and 'Set-Cookie'. Use the `getMultiple` > method to iterate the header values for these". Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:69: Stream<List<int>> get body; On 2014/05/23 21:22:24, kevmoo wrote: > Use a method here: read() > > In many cases, the body can only be accessed once. Documentation should > highlight this. > > See > https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/shelf/lib/src... Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:69: Stream<List<int>> get body; On 2014/05/23 21:22:24, kevmoo wrote: > Use a method here: read() > > In many cases, the body can only be accessed once. Documentation should > highlight this. > > See > https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/shelf/lib/src... Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:80: int get statusCode; On 2014/05/26 07:12:46, Søren Gjesse wrote: > If we remove reasonPhrase lets change this to just 'status'. In SPDY and > HTTP/2.0 it is the header-field name ':status'. Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:80: int get statusCode; On 2014/05/26 07:12:46, Søren Gjesse wrote: > If we remove reasonPhrase lets change this to just 'status'. In SPDY and > HTTP/2.0 it is the header-field name ':status'. Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:85: String get reasonPhrase; On 2014/05/26 07:12:46, Søren Gjesse wrote: > On 2014/05/23 21:22:24, kevmoo wrote: > > Can we skip this? > > > > It's not supported in HTTP 2.0 - > http://http2.github.io/http2-spec/#HttpHeaders > > > > We could create an class in a helper package (http_parser?) that has exposes > the > > Map<int, String> for each StatusCode > > I am OK with that - I does not add much value. > > The HTTP status code enumeration could be in this package - it is pretty basic. > We already have it once in dart:io. Lets think about it before we add to much > here for now. > > For HTTP 1.1 and SPDY a server should add a reason phrase. It can be empty, but > can cause issues if it is. Removed. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:95: Stream<List<int>> get body; On 2014/05/23 21:22:24, kevmoo wrote: > read() here, too. Same as above. Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:101: * Function for performing an http request. On 2014/05/23 12:25:30, Søren Gjesse wrote: > http -> HTTP > > Here is says 'an HTTP', and other places 'a HTTP'. It should be consistent. Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:116: typedef Future<Response> HttpClient(Request request); On 2014/05/26 07:12:46, Søren Gjesse wrote: > On 2014/05/23 21:22:24, kevmoo wrote: > > We must not export two identical typedefs. Not worth the confusion. > > > > Happy to have a conversation about a name that works well on both sides. > > > > For now, I'd argue that we get rid of HttpRequestHandler > > I agree that this can be confusing. However the "client" side will be sending > the request over some network transport having some server actually handle the > request. A "client" might "decorate" the request object to handle e.g. > authentication before sending the request. > > The "server" side is supposed to actually perform the processing of the request. > > HttpClient could just be Client. However a client is not really handling the > request - some server is. Removed the prefix, users should use prefix imports (since the name Client is very ambiguous). I agree with Soeren: These typedef names will be used by functions, and it makes it much clearer (in terms of documentation) whether something should be a HttpClient or something is a HttpRequestHandler. I'll keep the two typedefs for now. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:119: * Function for handling an http request. On 2014/05/23 12:25:30, Søren Gjesse wrote: > http -> HTTP Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:119: * Function for handling an http request. On 2014/05/23 12:25:30, Søren Gjesse wrote: > http -> HTTP Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:123: * ignored by an Http Server. On 2014/05/23 12:25:30, Søren Gjesse wrote: > Http -> HTTP Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:123: * ignored by an Http Server. On 2014/05/23 12:25:30, Søren Gjesse wrote: > Http -> HTTP Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:126: * another [HttpRequestHandler] and augment the headers with a 'Set-Cookie' On 2014/05/23 12:25:30, Søren Gjesse wrote: > with -> with e.g. Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:126: * another [HttpRequestHandler] and augment the headers with a 'Set-Cookie' On 2014/05/23 12:25:30, Søren Gjesse wrote: > with -> with e.g. Done. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/lib/h... dart/pkg/http_base/lib/http_base.dart:132: typedef Future<Response> HttpRequestHandler(Request request); On 2014/05/23 22:07:05, kevmoo wrote: > Eliminate this typedef. We only have confusion if we have two identical types. Not done. See above. https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/pubsp... File dart/pkg/http_base/pubspec.yaml (right): https://codereview.chromium.org/299443013/diff/20001/dart/pkg/http_base/pubsp... dart/pkg/http_base/pubspec.yaml:6: A library with common interfaces for http request objects, http response On 2014/05/23 12:25:30, Søren Gjesse wrote: > http -> HTTP Done.
Message was sent while issue was closed.
Committed patchset #3 manually as r36622 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
