pkg/http tweaks

pkg/http/lib/src/multipart_request.dart

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library multipart_request;
 
 import 'dart:async';
-import 'dart:io';
 import 'dart:math';
 import 'dart:utf';
 
 import 'base_request.dart';
 import 'byte_stream.dart';
 import 'multipart_file.dart';
 import 'utils.dart';
 
 /// A `multipart/form-data` request. Such a request has both string [fields],
 /// which function as normal form fields, and (potentially streamed) binary
@@ skipping 10 matching lines @@
 ///         'package',
 ///         new File('build/package.tar.gz'),
 ///         contentType: new ContentType('application', 'x-tar'));
 ///     request.send().then((response) {
 ///       if (response.statusCode == 200) print("Uploaded!");
 ///     });
 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();
 
   /// The form fields to send for this request.
   final Map<String, String> fields;
 
   /// The private version of [files].
   final List<MultipartFile> _files;
 
   /// Creates a new [MultipartRequest].
@@ skipping 18 matching lines @@
 
     for (var file in _files) {
       length += "--".length + _BOUNDARY_LENGTH + "\r\n".length +
           _headerForFile(file).length +
           file.length + "\r\n".length;
     }
 
     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() {
     // TODO(nweiz): freeze fields and files
     var boundary = _boundaryString(_BOUNDARY_LENGTH);
     headers['content-type'] = 'multipart/form-data; boundary="$boundary"';
     headers['content-transfer-encoding'] = 'binary';
     super.finalize();
 
     var controller = new StreamController<List<int>>(sync: true);
@@ skipping 23 matching lines @@
       // the stream. See issue 3657.
       writeAscii('--$boundary--\r\n');
       controller.close();
     });
 
     return new ByteStream(controller.stream);
   }
 
   /// 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,
     104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118,
     119, 120, 121, 122
   ];
 
   /// 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.
     var header =
         'content-disposition: form-data; name="${Uri.encodeFull(name)}"';
     if (!isPlainAscii(value)) {
       header = '$header\r\ncontent-type: text/plain; charset=utf-8';
     }
     return '$header\r\n\r\n';
   }
 
   /// 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)}"';
 
     if (file.filename != null) {
       header = '$header; filename="${Uri.encodeFull(file.filename)}"';
     }
     return '$header\r\n\r\n';
   }
 
   /// 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[
           _random.nextInt(_BOUNDARY_CHARACTERS.length)];
     }
     return "$prefix${new String.fromCharCodes(list)}";
   }
 }