Make BaseRequest.contentType use null rather than -1 as a flag value.

pkg/http/lib/src/base_request.dart

 // Copyright (c) 2012, 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 base_request;
 
 import 'dart:async';
 import 'dart:collection';
 
 import 'byte_stream.dart';
 import 'client.dart';
 import 'streamed_response.dart';
 import 'utils.dart';
 
 /// The base class for HTTP requests.
 ///
 /// Subclasses of [BaseRequest] can be constructed manually and passed to
 /// [BaseClient.send], which allows the user to provide fine-grained control
 /// over the request properties. However, usually it's easier to use convenience
 /// methods like [get] or [BaseClient.get].
 abstract class BaseRequest {
   /// The HTTP method of the request. Most commonly "GET" or "POST", less
   /// commonly "HEAD", "PUT", or "DELETE". Non-standard method names are also
   /// supported.
   final String method;
 
   /// The URL to which the request will be sent.
   final Uri url;
 
-  /// The size of the request body, in bytes. This defaults to -1, which
-  /// indicates that the size of the request is not known in advance.
+  /// The size of the request body, in bytes.
+  ///
+  /// This defaults to `null`, which indicates that the size of the request is

[Bob Nystrom, 2014/03/17 17:59:19]

Why this change?

I think most users expect ints to be non-nullable and other core APIs that
return ints use -1 as a "not found value", like String.indexOf().

[nweiz, 2014/03/17 19:36:01]

[String.indexOf] is a holdover from JavaScript which is in turn a holdover from
Java not having nullable ints. We do have nullable ints, so we should use them
to indicate the absence of an integer value rather than using the coincidental
property that a negative value is invalid in that context.

To put it another way: I'd rather make this consistent with other (non-int) dart
APIs' conventions for indicating that a value doesn't exist than with Java,
JavaScript, and [String.indexOf].

[Bob Nystrom, 2014/03/17 21:08:45]

Sure, and millions of people know and expect that behavior.
So there's a set of invalid values that this getter won't return: null and all
of the negative numbers. Given that other methods that return an int or some
invalid value use -1 for that, why do something different here?
Are you aware of any other *int* Dart APIs that do this? It seems like that's
the most relevant comparison here.

[nweiz, 2014/03/17 21:25:09]

Every JavaScript developer expects objects to be maps. Every Java developer
expects strict typing. We're a new language and there are some things we can do
better than old languages. This is one of them.
I disagree. This should be consistent with other APIs for accessing parsed
headers, not with a String API that happens to share a return type. When I add
more header-parsing APIs (which I intend to for consistency with Shelf), they
will return `null` if the header doesn't exist, because they won't
coincidentally return a scalar type with an unused range.

[kevmoo, 2014/03/17 21:30:53]

I agree with this logic.

For helpers accessing the Headers map: null for not set, otherwise the parsed
value.

[Bob Nystrom, 2014/03/17 21:32:05]

Sure, and Dart was designed expressly to not surprise the latter as much as
possible.
What's better about returning null here?
List.indexOf() also returns -1 on not found. I understand the principle here,
but I think consistency with the core libraries will make users' lives better
more than taking advantage of nullability. We've seen repeated feedback that
users *don't* intuit that the lowercase types in Dart or nullable. Whether they
should or not is a valid question, but it's clear many users would be
unpleasantly surprised by null here.

Can you point me to an existing Dart API that returns an int and uses null for
an invalid value where -1 could be used?

[nweiz, 2014/03/17 21:57:48]

Dart was designed in the tradition of Java and JavaScript, and its core
libraries show it. The APIs are verbose and difficult to use. The HTTP package
takes its inspiration from a different tradition, one that produces easy-to-use
APIs. This is one of many examples of things these APIs do differently than the
core Dart APIs, and like the others it increases usability at the expense of
unfamiliarity to users expecting something identical to Java.
It's consistent with other similar APIs, it will break if accidentally used as
an integer, and it communicates that the value doesn't exist where -1 could be
the result of a bogus header from the server (for a response).
I think there's no chance that "many users" will be surprised by this. I think
there's almost no chance that "many users" will touch this API at all; knowing
the content length ahead of time is not generally useful for user code. Some of
the people who do touch it will read the documentation; for them it will work
fine. Some of the remainder will share Kevin's and my intuition that this will
return null; for them it will work fine. Some of the remainder won't consider
the possibility that Content-Length is unset at all; for them it will break in a
much better way than it would if it returned -1. Only users who fall in none of
these buckets will be unpleasantly surprised in the way you describe.
I've already explained why I don't consider this relevant.

There are three levels of consistency here: consistency with the immediately
surrounding code; consistency with other functions with the same return type;
and consistency with all functions. In both the first and last levels, `null` is
used to represent an absence of a value. Only in the second level is `-1` used,
and then only in maybe half a dozen places. Why do you consider that layer to be
the most important?

+  /// not known in advance.
   int get contentLength => _contentLength;
-  int _contentLength = -1;
+  int _contentLength;
 
   set contentLength(int value) {
+    if (value < 0) throw new ArgumentError("Invalid content length $value.");

[kevmoo, 2014/03/17 17:53:25]

Is setting null valid here? Will the `<` blow up on null? (I'm not sure...)

Worth a test?

[nweiz, 2014/03/17 19:36:01]

Good catch. Done.

     _checkFinalized();
     _contentLength = value;
   }
 
   /// Whether a persistent connection should be maintained with the server.
   /// Defaults to true.
   bool get persistentConnection => _persistentConnection;
   bool _persistentConnection = true;
 
   set persistentConnection(bool value) {
@@ skipping 60 matching lines @@
   /// the request is complete. If you're planning on making multiple requests to
   /// the same server, you should use a single [Client] for all of those
   /// requests.
   Future<StreamedResponse> send() {
     var client = new Client();
     return client.send(this).then((response) {
       var stream = onDone(response.stream, client.close);
       return new StreamedResponse(
           new ByteStream(stream),
           response.statusCode,
-          response.contentLength,
+          response.contentLength == null ? -1 : response.contentLength,
           request: response.request,
           headers: response.headers,
           isRedirect: response.isRedirect,
           persistentConnection: response.persistentConnection,
           reasonPhrase: response.reasonPhrase);
     }).catchError((e) {
       client.close();
       throw e;
     });
   }
 
   // Throws an error if this request has been finalized.
   void _checkFinalized() {
     if (!finalized) return;
     throw new StateError("Can't modify a finalized Request.");
   }
 
   String toString() => "$method $url";
 }