pkg/http tweaks

pkg/http/lib/src/multipart_request.dart

Index: pkg/http/lib/src/multipart_request.dart
diff --git a/pkg/http/lib/src/multipart_request.dart b/pkg/http/lib/src/multipart_request.dart
index 051ac76b5701d1054f2cc8791991e7ed44414705..82c62549496349d7f4d0ef3a081e4f0268759643 100644
--- a/pkg/http/lib/src/multipart_request.dart
+++ b/pkg/http/lib/src/multipart_request.dart
@@ -5,7 +5,6 @@
 library multipart_request;
 
 import 'dart:async';
-import 'dart:io';
 import 'dart:math';
 import 'dart:utf';
 
@@ -36,7 +35,7 @@ class MultipartRequest extends BaseRequest {
   /// The total length of the multipart boundaries used when building the
   /// request body. According to http://tools.ietf.org/html/rfc1341.html, this
   /// can't be longer than 70.
-  static final int _BOUNDARY_LENGTH = 70;
+  static const int _BOUNDARY_LENGTH = 70;
 
   static final Random _random = new Random();
 
@@ -75,11 +74,6 @@ class MultipartRequest extends BaseRequest {
     return length + "--".length + _BOUNDARY_LENGTH + "--\r\n".length;
   }
 
-  set contentLength(int value) {
-    throw new UnsupportedError("Cannot set the contentLength property of "
-        "multipart requests.");
-  }

[nweiz, 2013/07/23 20:18:49]

Why is this being removed? It shouldn't be possible to directly set the
contentLength of multipart requests.

[kevmoo-old, 2013/07/23 21:08:56]

Didn't read the type hierarchy correctly. Oops.

-
   /// Freezes all mutable fields and returns a single-subscription [ByteStream]
   /// that will emit the request body.
   ByteStream finalize() {
@@ -123,7 +117,7 @@ class MultipartRequest extends BaseRequest {
 
   /// All character codes that are valid in multipart boundaries. From
   /// http://tools.ietf.org/html/rfc2046#section-5.1.1.
-  static final List<int> _BOUNDARY_CHARACTERS = const <int>[
+  static const List<int> _BOUNDARY_CHARACTERS = const <int>[
     39, 40, 41, 43, 95, 44, 45, 46, 47, 58, 61, 63, 48, 49, 50, 51, 52, 53, 54,
     55, 56, 57, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80,
     81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 97, 98, 99, 100, 101, 102, 103,
@@ -133,7 +127,7 @@ class MultipartRequest extends BaseRequest {
 
   /// Returns the header string for a field. The return value is guaranteed to
   /// contain only ASCII characters.
-  String _headerForField(String name, String value) {
+  static String _headerForField(String name, String value) {

[nweiz, 2013/07/23 20:18:49]

I'm not a fan of making methods static just because they happen to access no
instance state. It adds a useless token and misleadingly communicates that
they'll be accessed from a static context.

[kevmoo-old, 2013/07/23 21:08:56]

I think it correctly communicates it *could* be accessed via a static state,
which making it clear that no instance state is modified, let alone accessed.

[nweiz, 2013/07/23 21:27:34]

It's an implementation detail that it doesn't access instance state. There could
be configuration added later that this cares about. If it were something that
was meant for use outside of the an instance of MultipartRequest, it would be in
utils. In fact, it's a helper method for [finalize], and should inhabit the same
scope as that method.

[kevmoo-old, 2013/07/23 21:36:37]

I'm cool w/ philosophical differences here. Not a big deal. :-)

     // http://tools.ietf.org/html/rfc2388 mandates some complex encodings for
     // field names and file names, but in practice user agents seem to just
     // URL-encode them so we do the same.
@@ -147,7 +141,7 @@ class MultipartRequest extends BaseRequest {
 
   /// Returns the header string for a file. The return value is guaranteed to
   /// contain only ASCII characters.
-  String _headerForFile(MultipartFile file) {
+  static String _headerForFile(MultipartFile file) {
     var header = 'content-type: ${file.contentType}\r\n'
       'content-disposition: form-data; name="${Uri.encodeFull(file.field)}"';
 
@@ -159,8 +153,8 @@ class MultipartRequest extends BaseRequest {
 
   /// Returns a randomly-generated multipart boundary string of the given
   /// [length].
-  String _boundaryString(int length) {
-    var prefix = "dart-http-boundary-";
+  static String _boundaryString(int length) {
+    const prefix = "dart-http-boundary-";

[nweiz, 2013/07/23 20:18:49]

Also not a fan of const local variables, for roughly the same reason that I
don't like final or type-annotated local variables: its extra text that provides
no value, since locals are inherently of limited scope anyway.

[kevmoo-old, 2013/07/23 21:08:56]

Fair.

     var list = new List<int>(length - prefix.length);
     for (var i = 0; i < list.length; i++) {
       list[i] = _BOUNDARY_CHARACTERS[