Add an HTTP library that wraps dart:io.

Add an HTTP library that wraps dart:io.

Committed: https://code.google.com/p/dart/source/detail?r=14496

[Bob Nystrom, 2012-10-31 01:17:44]

Lots of comments, but overall I really really like this. Can't wait to start
using it in pub.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart
File pkg/http/lib/http.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart#newcode12
pkg/http/lib/http.dart:12: ///     var uri = new
Uri.fromString("http://example.com/whatsit/create");
This makes me wonder if the top-level functions should accept strings for the
first argument too.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart#newcode20
pkg/http/lib/http.dart:20: ///         .then((body) {
For maximum terseness:

.then(print);

:)

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart#newcode26
pkg/http/lib/http.dart:26: /// requests:
Add, "If you do this, make sure to close the client when you are done."

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart#newcode74
pkg/http/lib/http.dart:74: export 'src/stream_response.dart';
This is really awesome.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
File pkg/http/lib/src/base_client.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:21: /// Send an HTTP HEAD request with the
given headers to the given URI.
"Send" -> "Sends" here and elsewhere. I think we use progressive tense for
method docs?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:24: Future<Response> head(Uri uri,
{Map<String, String> headers: null}) =>
The ": null" shouldn't be required here and elsewhere.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:25: _sendNoStream("HEAD", uri, headers);
Since this is a line continuation, it should be indented +4. But I do like that
this isn't indented as far as the parameter lists in the methods below.

Thoughts?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:99: Future<Response> _sendNoStream(
This name feels a bit strange to me. Maybe "_sendUnstreamed" or
"_sendNonstreamed"?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:102: Map<String, String> headers,
Nit, but how about having the required params all on one line?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:106: return new
Future.immediate(null).chain((_) {
This can still cause errors to be sent synchronously. Do you think this should
explicitly pump the message queue to ensure it's consistently async?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:123: throw new HttpException("$message.");
Pub has its own PubHttpException class specifically to expose the reason and
status code as properties. How about copying that here (and then ditching the
pub one when pub uses this)?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_request...
File pkg/http/lib/src/base_request.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_request...
pkg/http/lib/src/base_request.dart:25: /// The URL to which request will be
sent.
"which request" -> "which the request".

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_request...
pkg/http/lib/src/base_request.dart:28: /// The size of the request body, in
bytes. This defaults to -1, which
Style nit. What do you think of putting literals like `-1` in code font?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_request...
pkg/http/lib/src/base_request.dart:81: /// This should not be called directly.
Only subclasses should be constructed.
It's a dynamic error now to try to instantiate an abstract class, so this
comment isn't needed.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_respons...
File pkg/http/lib/src/base_response.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_respons...
pkg/http/lib/src/base_response.dart:38: /// This should not be called directly.
Only subclasses should be constructed.
You can remove this comment.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_respons...
pkg/http/lib/src/base_response.dart:45: this.reasonPhrase: null});
Ditch ": null".

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/request.dart
File pkg/http/lib/src/request.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/request.dart...
pkg/http/lib/src/request.dart:42: /// will throw an [UnsupportedError].
UnsupportedError feels wrong here. Probably FormatError would be better?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/request.dart...
pkg/http/lib/src/request.dart:110: throw new UnsupportedError('Cannot access the
body fields of a Request '
Should be StateError I think.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/stream_reque...
File pkg/http/lib/src/stream_request.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/stream_reque...
pkg/http/lib/src/stream_request.dart:15: /// ended.
"will be ended" -> "will end".

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/stream_reque...
pkg/http/lib/src/stream_request.dart:16: class StreamRequest extends BaseRequest
{
This reads like a request for a stream and not a request where the request
itself is streamed. How about StreamedRequest?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/stream_respo...
File pkg/http/lib/src/stream_response.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/stream_respo...
pkg/http/lib/src/stream_response.dart:13: class StreamResponse extends
BaseResponse {
"StreamedResponse"?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:13: /// into a [Map] from parameter names to values.
Show an example here and elsewhere. In general, I'd like to see us use more API
examples in doc comments. Users request it a lot.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:30: pairs.add([encodeUriComponent(key),
encodeUriComponent(value)]));
Nit: Indent +2. If it were me, I would probably leave pairs.add([ on the
previous line too, but whatevs.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:37: source.forEach((key, value) => destination[key]
= value);
+2 here and elsewhere.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:52: toSplit.substring(index + pattern.length)];
+2.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:70: throw new UnsupportedError('Unsupported encoding
"$charset".');
I think this should be ArgumentError or possibly FormatError. If you expect
users to catch this, it should be FormatError. Otherwise, I think ArgumentError
is a better fit.

UnsupportedError is for "this object doesn't support that API" errors. Think
calling .add() on a read-only collection.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:98: Uint8List uint8List(List<int> input) {
This name isn't very verby. How about "toUint8List"?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:101: if (input is ByteArray) return new
Uint8List.view(input);
Does the order of these two lines matter? If not, I would prefer the early
returns to all come first.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:114: stream.onError = (e) =>
completer.completeException(e);
stream.onError = completer.completeException;

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:119: void pipeInputToInput(InputStream source,
ListInputStream sink) {
This method name seems crazy, but then I guess those type names are too. What is
up with this? They are both inputs?

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/http_test.dart
File pkg/http/test/http_test.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/http_test.dart#...
pkg/http/test/http_test.dart:14: group('http.', () {
These tests are great, but how about some failure tests too?

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/http_test.dart#...
pkg/http/test/http_test.dart:55: 'content-type':
['application/x-www-form-urlencoded; charset=UTF-8'],
Long line.

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/http_test.dart#...
pkg/http/test/http_test.dart:157: test('readBytes', () {
Add a test that this completes to an HttpException if the request fails.

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/request_test.dart
File pkg/http/test/request_test.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/request_test.da...
pkg/http/test/request_test.dart:300: test('freezes #maxRedirects', () {
Dupe of previous test.

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/utils.dart
File pkg/http/test/utils.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/utils.dart#newc...
pkg/http/test/utils.dart:109: const isStateError = const _StateError();
These should go away once unittest is updated to the new error types. Add a TODO
to remove them later.

[nweiz, 2012-10-31 18:20:58]

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart
File pkg/http/lib/http.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart#newcode12
pkg/http/lib/http.dart:12: ///     var uri = new
Uri.fromString("http://example.com/whatsit/create");
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> This makes me wonder if the top-level functions should accept strings for the
> first argument too.

That makes the type signature pretty ugly, but I guess it's probably worth it
for usability. What are your thoughts on adding a "Url" or "StringOrUri" typedef
for better documentation?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart#newcode20
pkg/http/lib/http.dart:20: ///         .then((body) {
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> For maximum terseness:
> 
> .then(print);
> 
> :)

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart#newcode26
pkg/http/lib/http.dart:26: /// requests:
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Add, "If you do this, make sure to close the client when you are done."

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
File pkg/http/lib/src/base_client.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:21: /// Send an HTTP HEAD request with the
given headers to the given URI.
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> "Send" -> "Sends" here and elsewhere. I think we use progressive tense for
> method docs?

Done. Guess I got too used to Python's style.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:24: Future<Response> head(Uri uri,
{Map<String, String> headers: null}) =>
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> The ": null" shouldn't be required here and elsewhere.

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:25: _sendNoStream("HEAD", uri, headers);
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Since this is a line continuation, it should be indented +4. But I do like
that
> this isn't indented as far as the parameter lists in the methods below.
> 
> Thoughts?

I like indenting this +2, since it reads more like the body of the method than
like a continued expression.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:99: Future<Response> _sendNoStream(
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> This name feels a bit strange to me. Maybe "_sendUnstreamed" or
> "_sendNonstreamed"?

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:102: Map<String, String> headers,
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Nit, but how about having the required params all on one line?

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:106: return new
Future.immediate(null).chain((_) {
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> This can still cause errors to be sent synchronously. Do you think this should
> explicitly pump the message queue to ensure it's consistently async?

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:123: throw new HttpException("$message.");
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Pub has its own PubHttpException class specifically to expose the reason and
> status code as properties. How about copying that here (and then ditching the
> pub one when pub uses this)?

I feel like that would be more confusing than it would be worth. The vast
majority of errors thrown by this package are still going to be HttpErrors;
you'd only get that particular HTTP error with status/reason when you use read
or readBytes in particular. It would almost never be correct to catch only it
and not also the underlying HTTP errors, and in the few cases where it is right
the user can use #get manually anyway.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_request...
File pkg/http/lib/src/base_request.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_request...
pkg/http/lib/src/base_request.dart:25: /// The URL to which request will be
sent.
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> "which request" -> "which the request".

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_request...
pkg/http/lib/src/base_request.dart:28: /// The size of the request body, in
bytes. This defaults to -1, which
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Style nit. What do you think of putting literals like `-1` in code font?

Dislike. I think it makes the docstring harder to read and doesn't add anything
to the compiled docs.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_request...
pkg/http/lib/src/base_request.dart:81: /// This should not be called directly.
Only subclasses should be constructed.
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> It's a dynamic error now to try to instantiate an abstract class, so this
> comment isn't needed.

Sweet.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_respons...
File pkg/http/lib/src/base_response.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_respons...
pkg/http/lib/src/base_response.dart:38: /// This should not be called directly.
Only subclasses should be constructed.
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> You can remove this comment.

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_respons...
pkg/http/lib/src/base_response.dart:45: this.reasonPhrase: null});
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Ditch ": null".

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/request.dart
File pkg/http/lib/src/request.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/request.dart...
pkg/http/lib/src/request.dart:42: /// will throw an [UnsupportedError].
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> UnsupportedError feels wrong here. Probably FormatError would be better?

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/request.dart...
pkg/http/lib/src/request.dart:110: throw new UnsupportedError('Cannot access the
body fields of a Request '
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Should be StateError I think.

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/stream_reque...
File pkg/http/lib/src/stream_request.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/stream_reque...
pkg/http/lib/src/stream_request.dart:15: /// ended.
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> "will be ended" -> "will end".

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/stream_reque...
pkg/http/lib/src/stream_request.dart:16: class StreamRequest extends BaseRequest
{
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> This reads like a request for a stream and not a request where the request
> itself is streamed. How about StreamedRequest?

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/stream_respo...
File pkg/http/lib/src/stream_response.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/stream_respo...
pkg/http/lib/src/stream_response.dart:13: class StreamResponse extends
BaseResponse {
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> "StreamedResponse"?

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:13: /// into a [Map] from parameter names to values.
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Show an example here and elsewhere. In general, I'd like to see us use more
API
> examples in doc comments. Users request it a lot.

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:30: pairs.add([encodeUriComponent(key),
encodeUriComponent(value)]));
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Nit: Indent +2. If it were me, I would probably leave pairs.add([ on the
> previous line too, but whatevs.

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:37: source.forEach((key, value) => destination[key]
= value);
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> +2 here and elsewhere.

As I mentioned elsewhere, I feel like this is more of a method body than I line
continuation.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:52: toSplit.substring(index + pattern.length)];
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> +2.

Changed the formatting in a different way, let me know what you think.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:70: throw new UnsupportedError('Unsupported encoding
"$charset".');
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> I think this should be ArgumentError or possibly FormatError. If you expect
> users to catch this, it should be FormatError. Otherwise, I think
ArgumentError
> is a better fit.
> 
> UnsupportedError is for "this object doesn't support that API" errors. Think
> calling .add() on a read-only collection.

Changed to ArgumentError.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:98: Uint8List uint8List(List<int> input) {
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> This name isn't very verby. How about "toUint8List"?

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:101: if (input is ByteArray) return new
Uint8List.view(input);
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Does the order of these two lines matter? If not, I would prefer the early
> returns to all come first.

Yes, the previous line causes "input" to become a byte array, which triggers
this line.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:114: stream.onError = (e) =>
completer.completeException(e);
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> stream.onError = completer.completeException;

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:119: void pipeInputToInput(InputStream source,
ListInputStream sink) {
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> This method name seems crazy, but then I guess those type names are too. What
is
> up with this? They are both inputs?

In the context of this method, "sink" is coneptually functioning as an output
stream. But dart:io doesn't have a concept of creating linked input/output
streams, it just has its janky ListInputStream and ListOutputStream classes.
Hence the crazy naming.

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/http_test.dart
File pkg/http/test/http_test.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/http_test.dart#...
pkg/http/test/http_test.dart:14: group('http.', () {
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> These tests are great, but how about some failure tests too?

I've added some tests for the HttpException that we raise from read() and
readBytes(), but I'm not sure it's correct to test for exceptions from
connection issues. Those exceptions are created by dart:io, and all we do is
propagate them (often implicitly). I think we'd just end up testing dart:io.

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/http_test.dart#...
pkg/http/test/http_test.dart:55: 'content-type':
['application/x-www-form-urlencoded; charset=UTF-8'],
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/http_test.dart#...
pkg/http/test/http_test.dart:157: test('readBytes', () {
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Add a test that this completes to an HttpException if the request fails.

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/request_test.dart
File pkg/http/test/request_test.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/request_test.da...
pkg/http/test/request_test.dart:300: test('freezes #maxRedirects', () {
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> Dupe of previous test.

Done.

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/utils.dart
File pkg/http/test/utils.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/test/utils.dart#newc...
pkg/http/test/utils.dart:109: const isStateError = const _StateError();
On 2012/10/31 01:17:44, Bob Nystrom wrote:
> These should go away once unittest is updated to the new error types. Add a
TODO
> to remove them later.

Done.

[Bob Nystrom, 2012-11-01 19:53:59]

Couple more suggestions/questions but almost there.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart
File pkg/http/lib/http.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart#newcode12
pkg/http/lib/http.dart:12: ///     var uri = new
Uri.fromString("http://example.com/whatsit/create");
On 2012/10/31 18:20:59, nweiz wrote:
> On 2012/10/31 01:17:44, Bob Nystrom wrote:
> > This makes me wonder if the top-level functions should accept strings for
the
> > first argument too.
> 
> That makes the type signature pretty ugly, but I guess it's probably worth it
> for usability. What are your thoughts on adding a "Url" or "StringOrUri"
typedef
> for better documentation?

I don't think we can use typedefs for this, at least not yet. As far as I know,
typedef is still just for function types right now. I guess we should probably
just leave it typed and then document that it can take a URI or a string.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
File pkg/http/lib/src/base_client.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:123: throw new HttpException("$message.");
On 2012/10/31 18:20:59, nweiz wrote:
> On 2012/10/31 01:17:44, Bob Nystrom wrote:
> > Pub has its own PubHttpException class specifically to expose the reason and
> > status code as properties. How about copying that here (and then ditching
the
> > pub one when pub uses this)?
> 
> I feel like that would be more confusing than it would be worth. The vast
> majority of errors thrown by this package are still going to be HttpErrors;
> you'd only get that particular HTTP error with status/reason when you use read
> or readBytes in particular.

Yeah, I definitely wouldn't want to mix and match error types. But could we
consistently return a single HTTP error type that has useful fields?

I made this change in pub's io.dart because there is code in pub that looks at
the status code to give a better error message. It's nice to have the exception
object expose that directly.

> It would almost never be correct to catch only it
> and not also the underlying HTTP errors, and in the few cases where it is
right
> the user can use #get manually anyway.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_respons...
File pkg/http/lib/src/base_response.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_respons...
pkg/http/lib/src/base_response.dart:42: {this.headers: const <String>{},
Hmm, does this mean that if you don't provide any headers on construction that
you can't add any later? Is that intended.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart
File pkg/http/lib/src/utils.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/utils.dart#n...
pkg/http/lib/src/utils.dart:52: toSplit.substring(index + pattern.length)];
On 2012/10/31 18:20:59, nweiz wrote:
> On 2012/10/31 01:17:44, Bob Nystrom wrote:
> > +2.
> 
> Changed the formatting in a different way, let me know what you think.

+1. I like it.

https://codereview.chromium.org/11338054/diff/13001/pkg/http/lib/src/request....
File pkg/http/lib/src/request.dart (right):

https://codereview.chromium.org/11338054/diff/13001/pkg/http/lib/src/request....
pkg/http/lib/src/request.dart:42: /// will throw an [ArgumentError].
ArgumentError is wrong here since getters don't have any arguments. It would
either be StateError (since this is based on a field having the wrong value) or
a FormatError. I lean towards the latter since this is an error that can occur
at runtime and that users may want to catch. (StateError is only for
programmatic errors.)

[nweiz, 2012-11-02 19:29:12]

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart
File pkg/http/lib/http.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart#newcode12
pkg/http/lib/http.dart:12: ///     var uri = new
Uri.fromString("http://example.com/whatsit/create");
On 2012/11/01 19:53:59, Bob Nystrom wrote:
> On 2012/10/31 18:20:59, nweiz wrote:
> > On 2012/10/31 01:17:44, Bob Nystrom wrote:
> > > This makes me wonder if the top-level functions should accept strings for
> the
> > > first argument too.
> > 
> > That makes the type signature pretty ugly, but I guess it's probably worth
it
> > for usability. What are your thoughts on adding a "Url" or "StringOrUri"
> typedef
> > for better documentation?
> 
> I don't think we can use typedefs for this, at least not yet. As far as I
know,
> typedef is still just for function types right now. I guess we should probably
> just leave it typed and then document that it can take a URI or a string.

By "leave it typed" do you mean "leave it untyped"?

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
File pkg/http/lib/src/base_client.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:123: throw new HttpException("$message.");
On 2012/11/01 19:53:59, Bob Nystrom wrote:
> Yeah, I definitely wouldn't want to mix and match error types. But could we
> consistently return a single HTTP error type that has useful fields?
> 
> I made this change in pub's io.dart because there is code in pub that looks at
> the status code to give a better error message. It's nice to have the
exception
> object expose that directly.

The thing is, most HTTP errors happen because of things like the response
failing to parse or some formatting error in the request. In most cases it
doesn't make sense to add extra metadata. Really the only case where it would is
for these convenience methods, and that raises the issues I described above.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_respons...
File pkg/http/lib/src/base_response.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_respons...
pkg/http/lib/src/base_response.dart:42: {this.headers: const <String>{},
On 2012/11/01 19:53:59, Bob Nystrom wrote:
> Hmm, does this mean that if you don't provide any headers on construction that
> you can't add any later? Is that intended.

Yes, that's intended. A response is supposed to be read-only.

https://codereview.chromium.org/11338054/diff/13001/pkg/http/lib/src/request....
File pkg/http/lib/src/request.dart (right):

https://codereview.chromium.org/11338054/diff/13001/pkg/http/lib/src/request....
pkg/http/lib/src/request.dart:42: /// will throw an [ArgumentError].
On 2012/11/01 19:53:59, Bob Nystrom wrote:
> ArgumentError is wrong here since getters don't have any arguments. It would
> either be StateError (since this is based on a field having the wrong value)
or
> a FormatError. I lean towards the latter since this is an error that can occur
> at runtime and that users may want to catch. (StateError is only for
> programmatic errors.)

Changed to FormatError.

[Bob Nystrom, 2012-11-02 19:35:03]

LGTM!

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart
File pkg/http/lib/http.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/http.dart#newcode12
pkg/http/lib/http.dart:12: ///     var uri = new
Uri.fromString("http://example.com/whatsit/create");
On 2012/11/02 19:29:12, nweiz wrote:
> On 2012/11/01 19:53:59, Bob Nystrom wrote:
> > On 2012/10/31 18:20:59, nweiz wrote:
> > > On 2012/10/31 01:17:44, Bob Nystrom wrote:
> > > > This makes me wonder if the top-level functions should accept strings
for
> > the
> > > > first argument too.
> > > 
> > > That makes the type signature pretty ugly, but I guess it's probably worth
> it
> > > for usability. What are your thoughts on adding a "Url" or "StringOrUri"
> > typedef
> > > for better documentation?
> > 
> > I don't think we can use typedefs for this, at least not yet. As far as I
> know,
> > typedef is still just for function types right now. I guess we should
probably
> > just leave it typed and then document that it can take a URI or a string.
> 
> By "leave it typed" do you mean "leave it untyped"?

Yup, sorry.

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
File pkg/http/lib/src/base_client.dart (right):

https://codereview.chromium.org/11338054/diff/1/pkg/http/lib/src/base_client....
pkg/http/lib/src/base_client.dart:123: throw new HttpException("$message.");
On 2012/11/02 19:29:12, nweiz wrote:
> On 2012/11/01 19:53:59, Bob Nystrom wrote:
> > Yeah, I definitely wouldn't want to mix and match error types. But could we
> > consistently return a single HTTP error type that has useful fields?
> > 
> > I made this change in pub's io.dart because there is code in pub that looks
at
> > the status code to give a better error message. It's nice to have the
> exception
> > object expose that directly.
> 
> The thing is, most HTTP errors happen because of things like the response
> failing to parse or some formatting error in the request. In most cases it
> doesn't make sense to add extra metadata. Really the only case where it would
is
> for these convenience methods, and that raises the issues I described above.

SGTM.