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

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

BUG=
R=rnystrom@google.com

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

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

DBC

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

https://codereview.chromium.org/196423017/diff/1/pkg/http/lib/src/base_reques...
pkg/http/lib/src/base_request.dart:38: if (value < 0) throw new
ArgumentError("Invalid content length $value.");
Is setting null valid here? Will the `<` blow up on null? (I'm not sure...)

Worth a test?

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

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

https://codereview.chromium.org/196423017/diff/1/pkg/http/lib/src/base_reques...
pkg/http/lib/src/base_request.dart:32: /// This defaults to `null`, which
indicates that the size of the request is
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]

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

https://codereview.chromium.org/196423017/diff/1/pkg/http/lib/src/base_reques...
pkg/http/lib/src/base_request.dart:32: /// This defaults to `null`, which
indicates that the size of the request is
On 2014/03/17 17:59:19, Bob Nystrom wrote:
> 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().

[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].

https://codereview.chromium.org/196423017/diff/1/pkg/http/lib/src/base_reques...
pkg/http/lib/src/base_request.dart:38: if (value < 0) throw new
ArgumentError("Invalid content length $value.");
On 2014/03/17 17:53:25, kevmoo wrote:
> Is setting null valid here? Will the `<` blow up on null? (I'm not sure...)
> 
> Worth a test?

Good catch. Done.

[Bob Nystrom, 2014-03-17 21:08:44]

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

https://codereview.chromium.org/196423017/diff/1/pkg/http/lib/src/base_reques...
pkg/http/lib/src/base_request.dart:32: /// This defaults to `null`, which
indicates that the size of the request is
On 2014/03/17 19:36:01, nweiz wrote:
> On 2014/03/17 17:59:19, Bob Nystrom wrote:
> > 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().
> 
> [String.indexOf] is a holdover from JavaScript which is in turn a holdover
from
> Java not having nullable ints.

Sure, and millions of people know and expect that behavior.

> 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.

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?

> To put it another way: I'd rather make this consistent with other (non-int)
dart
> APIs' conventions

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]

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

https://codereview.chromium.org/196423017/diff/1/pkg/http/lib/src/base_reques...
pkg/http/lib/src/base_request.dart:32: /// This defaults to `null`, which
indicates that the size of the request is
On 2014/03/17 21:08:45, Bob Nystrom wrote:
> On 2014/03/17 19:36:01, nweiz wrote:
> > On 2014/03/17 17:59:19, Bob Nystrom wrote:
> > > 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().
> > 
> > [String.indexOf] is a holdover from JavaScript which is in turn a holdover
> from
> > Java not having nullable ints.
> 
> Sure, and millions of people know and expect that behavior.

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.

> > 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.
> 
> 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?
> 
> > To put it another way: I'd rather make this consistent with other (non-int)
> dart
> > APIs' conventions
> 
> Are you aware of any other *int* Dart APIs that do this? It seems like that's
> the most relevant comparison here.

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]

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

https://codereview.chromium.org/196423017/diff/1/pkg/http/lib/src/base_reques...
pkg/http/lib/src/base_request.dart:32: /// This defaults to `null`, which
indicates that the size of the request is
> 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.

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]

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

https://codereview.chromium.org/196423017/diff/1/pkg/http/lib/src/base_reques...
pkg/http/lib/src/base_request.dart:32: /// This defaults to `null`, which
indicates that the size of the request is
On 2014/03/17 21:25:09, nweiz wrote:
> On 2014/03/17 21:08:45, Bob Nystrom wrote:
> > On 2014/03/17 19:36:01, nweiz wrote:
> > > On 2014/03/17 17:59:19, Bob Nystrom wrote:
> > > > 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().
> > > 
> > > [String.indexOf] is a holdover from JavaScript which is in turn a holdover
> > from
> > > Java not having nullable ints.
> > 
> > Sure, and millions of people know and expect that behavior.
> 
> Every JavaScript developer expects objects to be maps. Every Java developer
> expects strict typing.

Sure, and Dart was designed expressly to not surprise the latter as much as
possible.

> We're a new language and there are some things we can do
> better than old languages. This is one of them.

What's better about returning null here?

> 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.

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:47]

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

https://codereview.chromium.org/196423017/diff/1/pkg/http/lib/src/base_reques...
pkg/http/lib/src/base_request.dart:32: /// This defaults to `null`, which
indicates that the size of the request is
On 2014/03/17 21:32:05, Bob Nystrom wrote:
> On 2014/03/17 21:25:09, nweiz wrote:
> > On 2014/03/17 21:08:45, Bob Nystrom wrote:
> > > On 2014/03/17 19:36:01, nweiz wrote:
> > > > On 2014/03/17 17:59:19, Bob Nystrom wrote:
> > > > > 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().
> > > > 
> > > > [String.indexOf] is a holdover from JavaScript which is in turn a
holdover
> > > from
> > > > Java not having nullable ints.
> > > 
> > > Sure, and millions of people know and expect that behavior.
> > 
> > Every JavaScript developer expects objects to be maps. Every Java developer
> > expects strict typing.
> 
> Sure, and Dart was designed expressly to not surprise the latter as much as
> possible.

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.

> > We're a new language and there are some things we can do
> > better than old languages. This is one of them.
> 
> What's better about returning null here?

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 disagree. This should be consistent with other APIs for accessing parsed
> > headers, not with a String API that happens to share a return type.
> 
> 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.

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.

> 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?

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?

[Bob Nystrom, 2014-03-17 22:03:09]

OK, LGTM.

[nweiz, 2014-03-17 22:14:30]

Committed patchset #2 manually as r33781 (presubmit successful).