pkg/shelf: change helper method on Request and Response

pkg/shelf: change helper method on Request and Response

BUG= https://code.google.com/p/dart/issues/detail?id=18453
R=nweiz@google.com

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

[kevmoo, 2014-04-26 16:20:57]

ALSO: I started making the 4th copy of the 'hello, world' bytes and decided it'd
be good to DRY 'em up.

[kevmoo, 2014-04-26 16:34:34]

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
File pkg/shelf/lib/src/request.dart (right):

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:87: if (requestedUri.scheme.isEmpty) {
The RFC is a bit weird about fragments, hence Uri.isAbsolute is weird.

Just checking for a scheme instead.

[nweiz, 2014-04-28 22:13:33]

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
File pkg/shelf/lib/src/request.dart (right):

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:86: // So instead of using Uri.isAbsolute, just
checking for an empty scheme
Use full sentences in comments.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:87: if (requestedUri.scheme.isEmpty) {
On 2014/04/26 16:34:34, kevmoo wrote:
> The RFC is a bit weird about fragments, hence Uri.isAbsolute is weird.
> 
> Just checking for a scheme instead.

I think we should use [isAbsolute] here. The requested URI should never include
a fragment identifier anyway.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:113: /// Copies the request updated [context] and
[headers] values.
Fix this sentence.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:124: Request copy({Map<String, String> headers,
Map<String, Object> context}) {
We should discuss this name a bit. I like "copy" okay, but it implies that the
primary purpose of this method is to make a copy of request. For MediaType I
used "change", which is a little longer but communicates that this is the way
you change the fields of the request.

Also, this should be able to change any properties exposed by the request,
although it's fine to do that in a followup CL.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:130: scriptName: this.scriptName, body:
this.read(), context: context);
I don't like that this copies the headers and re-wraps the context map even when
they're not modified at all. I suggest you create a _ShelfUnmodifiableMapView
that extends UnmodifiableMapView, use that to wrap [headers] and [context], and
check for it in the Message constructor to see if you can avoid re-copying
and/or re-wrapping.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:134: Map _updatedMap(Map original, Map updates) {
Functions should have verb names, e.g. "_updateMap". Also, document this.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/test/request_t...
File pkg/shelf/test/request_test.dart (right):

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/test/request_t...
pkg/shelf/test/request_test.dart:215: test('with empty headers returns
indentical instance', () {
"indentical" -> "identical"

This also doesn't test for an identical instance, nor is it in fact identical
since the headers are copied into a new IgnoreCaseMapView. It's just equal.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/test/request_t...
pkg/shelf/test/request_test.dart:273: const _WORLD_BYTES = const [32, 119, 111,
114, 108, 100];
Constants go at the top of the file.

[kevmoo, 2014-04-29 10:41:53]

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
File pkg/shelf/lib/src/request.dart (right):

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:86: // So instead of using Uri.isAbsolute, just
checking for an empty scheme
On 2014/04/28 22:13:33, nweiz wrote:
> Use full sentences in comments.

Done.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:87: if (requestedUri.scheme.isEmpty) {
On 2014/04/28 22:13:33, nweiz wrote:
> On 2014/04/26 16:34:34, kevmoo wrote:
> > The RFC is a bit weird about fragments, hence Uri.isAbsolute is weird.
> > 
> > Just checking for a scheme instead.
> 
> I think we should use [isAbsolute] here. The requested URI should never
include
> a fragment identifier anyway.

I double checked. dart_io doesn't send the fragment. Done.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:113: /// Copies the request updated [context] and
[headers] values.
On 2014/04/28 22:13:33, nweiz wrote:
> Fix this sentence.

Done.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:124: Request copy({Map<String, String> headers,
Map<String, Object> context}) {
On 2014/04/28 22:13:33, nweiz wrote:
> We should discuss this name a bit. I like "copy" okay, but it implies that the
> primary purpose of this method is to make a copy of request. For MediaType I
> used "change", which is a little longer but communicates that this is the way
> you change the fields of the request.
> 
> Also, this should be able to change any properties exposed by the request,
> although it's fine to do that in a followup CL.

Done.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:130: scriptName: this.scriptName, body:
this.read(), context: context);
On 2014/04/28 22:13:33, nweiz wrote:
> I don't like that this copies the headers and re-wraps the context map even
when
> they're not modified at all. I suggest you create a _ShelfUnmodifiableMapView
> that extends UnmodifiableMapView, use that to wrap [headers] and [context],
and
> check for it in the Message constructor to see if you can avoid re-copying
> and/or re-wrapping.

Done.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:134: Map _updatedMap(Map original, Map updates) {
On 2014/04/28 22:13:33, nweiz wrote:
> Functions should have verb names, e.g. "_updateMap". Also, document this.

Done.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/test/request_t...
File pkg/shelf/test/request_test.dart (right):

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/test/request_t...
pkg/shelf/test/request_test.dart:215: test('with empty headers returns
indentical instance', () {
On 2014/04/28 22:13:33, nweiz wrote:
> "indentical" -> "identical"
> 
> This also doesn't test for an identical instance, nor is it in fact identical
> since the headers are copied into a new IgnoreCaseMapView. It's just equal.

Done.

https://codereview.chromium.org/256753004/diff/20001/pkg/shelf/test/request_t...
pkg/shelf/test/request_test.dart:273: const _WORLD_BYTES = const [32, 119, 111,
114, 108, 100];
On 2014/04/28 22:13:33, nweiz wrote:
> Constants go at the top of the file.

Done.

[nweiz, 2014-04-29 19:48:48]

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
File pkg/shelf/lib/src/message.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:127: Map updateMap(Map original, Map updates) {
Put this in util.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:137: /// If [source] is a [_ShelfUnmodifiableMap]
with matcher [ignoreKeyCase], then
"matcher" -> "matching"

This and the following text should be on the constructor, since they're about
how constructor arguments are handled.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:146: class _ShelfUnmodifiableMap<V> extends
pc.UnmodifiableMapView<String, V> {
This should probably go in its own library.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:147: final bool ignoreKeyCase;
Document this field.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:149: factory _ShelfUnmodifiableMap(Map<String, V>
source, bool ignoreKeyCase) {
Make [ignoreKeyCase] a named argument so it's clearer at the call site.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:156: return new _ShelfUnmodifiableMap<V>._(const
{}, ignoreKeyCase);
It would be really nice to be able to just return "const {}" here so we can
avoid any extra object allocations for the common case of an empty context map.
Maybe instead of a factory constructor, this should be a static method?

    static Map<String, dynamic> wrap(Map<String, dynamic> source, {bool
ignoreKeyCase});

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/request_t...
File pkg/shelf/test/request_test.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/request_t...
pkg/shelf/test/request_test.dart:173: testChange(({headers, context}) {
Why are these named parameters?

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/response_...
File pkg/shelf/test/response_test.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/response_...
pkg/shelf/test/response_test.dart:124: test('with no arguments returns instance
with equal values', () {
Why isn't this part of [testChange]?

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/test_chan...
File pkg/shelf/test/test_change.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/test_chan...
pkg/shelf/test/test_change.dart:13: void testChange(Message factory({Map<String,
String> headers,
Rather than calling this method in request_test and response_test, I think it'd
be cleaner to just have this be its own test file that iterates over the two
classes and defines tests for each.

[kevmoo, 2014-04-30 14:27:51]

PTAL

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
File pkg/shelf/lib/src/message.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:127: Map updateMap(Map original, Map updates) {
On 2014/04/29 19:48:48, nweiz wrote:
> Put this in util.

Neither Request nor Response import util at the moment.

But they both import Message. Seems okay to keep msg-specific helpers here IMHO

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:137: /// If [source] is a [_ShelfUnmodifiableMap]
with matcher [ignoreKeyCase], then
On 2014/04/29 19:48:48, nweiz wrote:
> "matcher" -> "matching"
> 
> This and the following text should be on the constructor, since they're about
> how constructor arguments are handled.

Done.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:146: class _ShelfUnmodifiableMap<V> extends
pc.UnmodifiableMapView<String, V> {
On 2014/04/29 19:48:48, nweiz wrote:
> This should probably go in its own library.

I like having it private here. It's purely an implementation detail of Message.
No possible way for someone to import it via pkg uri silly.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:147: final bool ignoreKeyCase;
On 2014/04/29 19:48:48, nweiz wrote:
> Document this field.

Done.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:149: factory _ShelfUnmodifiableMap(Map<String, V>
source, bool ignoreKeyCase) {
On 2014/04/29 19:48:48, nweiz wrote:
> Make [ignoreKeyCase] a named argument so it's clearer at the call site.

Done.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:156: return new _ShelfUnmodifiableMap<V>._(const
{}, ignoreKeyCase);
On 2014/04/29 19:48:48, nweiz wrote:
> It would be really nice to be able to just return "const {}" here so we can
> avoid any extra object allocations for the common case of an empty context
map.
> Maybe instead of a factory constructor, this should be a static method?
> 
>     static Map<String, dynamic> wrap(Map<String, dynamic> source, {bool
> ignoreKeyCase});

Take a look at my alternative. We keep the generic types and get nice const
behavior

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/request_t...
File pkg/shelf/test/request_test.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/request_t...
pkg/shelf/test/request_test.dart:173: testChange(({headers, context}) {
On 2014/04/29 19:48:48, nweiz wrote:
> Why are these named parameters?

They align exactly with the shared ctor signature.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/response_...
File pkg/shelf/test/response_test.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/response_...
pkg/shelf/test/response_test.dart:124: test('with no arguments returns instance
with equal values', () {
On 2014/04/29 19:48:48, nweiz wrote:
> Why isn't this part of [testChange]?

Because I'm ensuring that all of the non-common fields are copied -- nothing was
forgotten in the custom change impl

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/test_chan...
File pkg/shelf/test/test_change.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/test_chan...
pkg/shelf/test/test_change.dart:13: void testChange(Message factory({Map<String,
String> headers,
On 2014/04/29 19:48:48, nweiz wrote:
> Rather than calling this method in request_test and response_test, I think
it'd
> be cleaner to just have this be its own test file that iterates over the two
> classes and defines tests for each.

I like the idea that these are relatively isolated. We don't run this file. It's
clear if Request or Response test stars to fail what is broken.

This is logically a helper, it just happens to define tests

[nweiz, 2014-04-30 20:21:38]

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
File pkg/shelf/lib/src/message.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:127: Map updateMap(Map original, Map updates) {
On 2014/04/30 14:27:52, kevmoo wrote:
> On 2014/04/29 19:48:48, nweiz wrote:
> > Put this in util.
> 
> Neither Request nor Response import util at the moment.

It's not onerous to add an import statement.

> But they both import Message. Seems okay to keep msg-specific helpers here
IMHO

This isn't message-specific. It's a helper that non-destructively adds elements
from one map to another. That's pretty general functionality, and so belongs in
util.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:146: class _ShelfUnmodifiableMap<V> extends
pc.UnmodifiableMapView<String, V> {
On 2014/04/30 14:27:52, kevmoo wrote:
> On 2014/04/29 19:48:48, nweiz wrote:
> > This should probably go in its own library.
> 
> I like having it private here. It's purely an implementation detail of
Message.

Shoving multiple classes into a single library when their implementations aren't
closely tied to one another tends to produce large and unfocused files. The
notion of an immutable map with an optimized constructor isn't related to the
notion of a message at all.

> No possible way for someone to import it via pkg uri silly.

Regardless of whether it's enforced by the language, external users aren't
allowed to import other packages' src/ libraries. Saying "but users could import
it" is by no means a good reason not to use multiple libraries in a package's
implementation.

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/test_chan...
File pkg/shelf/test/test_change.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/test_chan...
pkg/shelf/test/test_change.dart:13: void testChange(Message factory({Map<String,
String> headers,
On 2014/04/30 14:27:52, kevmoo wrote:
> On 2014/04/29 19:48:48, nweiz wrote:
> > Rather than calling this method in request_test and response_test, I think
> it'd
> > be cleaner to just have this be its own test file that iterates over the two
> > classes and defines tests for each.
> 
> I like the idea that these are relatively isolated. We don't run this file.
It's
> clear if Request or Response test stars to fail what is broken.
> 
> This is logically a helper, it just happens to define tests

It's confusing to read. The flow of control bounces back and forth between
files, making it very difficult to follow. It's unclear when looking at the
request/response tests what tests are being run, and it's unclear when looking
at this file what the tests are being run on. It also makes the test files more
coarse-grained which can make debugging more difficult.

When writing a package that exposes a few classes with a lot of functionality,
the pattern is usually to separate test files based on the features those
classes expose. You should do so here rather than bending over backwards to only
have one test file per class.

https://codereview.chromium.org/256753004/diff/120001/pkg/shelf/lib/src/messa...
File pkg/shelf/lib/src/message.dart (right):

https://codereview.chromium.org/256753004/diff/120001/pkg/shelf/lib/src/messa...
pkg/shelf/lib/src/message.dart:140: final bool ignoreKeyCase;
This shouldn't be public.

https://codereview.chromium.org/256753004/diff/120001/pkg/shelf/lib/src/messa...
pkg/shelf/lib/src/message.dart:186: final bool ignoreKeyCase;
Once [ignoreKeyCase] is no longer public, there's no reason for this to have the
property at all.

[kevmoo, 2014-05-02 16:18:01]

PTAL

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
File pkg/shelf/lib/src/message.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:127: Map updateMap(Map original, Map updates) {
On 2014/04/30 20:21:39, nweiz wrote:
> On 2014/04/30 14:27:52, kevmoo wrote:
> > On 2014/04/29 19:48:48, nweiz wrote:
> > > Put this in util.
> > 
> > Neither Request nor Response import util at the moment.
> 
> It's not onerous to add an import statement.
> 
> > But they both import Message. Seems okay to keep msg-specific helpers here
> IMHO
> 
> This isn't message-specific. It's a helper that non-destructively adds
elements
> from one map to another. That's pretty general functionality, and so belongs
in
> util.

*grumble* Done *eye roll*

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/lib/src/messag...
pkg/shelf/lib/src/message.dart:146: class _ShelfUnmodifiableMap<V> extends
pc.UnmodifiableMapView<String, V> {
On 2014/04/30 20:21:39, nweiz wrote:
> On 2014/04/30 14:27:52, kevmoo wrote:
> > On 2014/04/29 19:48:48, nweiz wrote:
> > > This should probably go in its own library.
> > 
> > I like having it private here. It's purely an implementation detail of
> Message.
> 
> Shoving multiple classes into a single library when their implementations
aren't
> closely tied to one another tends to produce large and unfocused files. The
> notion of an immutable map with an optimized constructor isn't related to the
> notion of a message at all.
> 
> > No possible way for someone to import it via pkg uri silly.
> 
> Regardless of whether it's enforced by the language, external users aren't
> allowed to import other packages' src/ libraries. Saying "but users could
import
> it" is by no means a good reason not to use multiple libraries in a package's
> implementation.

fine fine Done yeah yeah

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/test_chan...
File pkg/shelf/test/test_change.dart (right):

https://codereview.chromium.org/256753004/diff/80001/pkg/shelf/test/test_chan...
pkg/shelf/test/test_change.dart:13: void testChange(Message factory({Map<String,
String> headers,
On 2014/04/30 20:21:39, nweiz wrote:
> On 2014/04/30 14:27:52, kevmoo wrote:
> > On 2014/04/29 19:48:48, nweiz wrote:
> > > Rather than calling this method in request_test and response_test, I think
> > it'd
> > > be cleaner to just have this be its own test file that iterates over the
two
> > > classes and defines tests for each.
> > 
> > I like the idea that these are relatively isolated. We don't run this file.
> It's
> > clear if Request or Response test stars to fail what is broken.
> > 
> > This is logically a helper, it just happens to define tests
> 
> It's confusing to read. The flow of control bounces back and forth between
> files, making it very difficult to follow. It's unclear when looking at the
> request/response tests what tests are being run, and it's unclear when looking
> at this file what the tests are being run on. It also makes the test files
more
> coarse-grained which can make debugging more difficult.
> 
> When writing a package that exposes a few classes with a lot of functionality,
> the pattern is usually to separate test files based on the features those
> classes expose. You should do so here rather than bending over backwards to
only
> have one test file per class.

Done.

https://codereview.chromium.org/256753004/diff/120001/pkg/shelf/lib/src/messa...
File pkg/shelf/lib/src/message.dart (right):

https://codereview.chromium.org/256753004/diff/120001/pkg/shelf/lib/src/messa...
pkg/shelf/lib/src/message.dart:140: final bool ignoreKeyCase;
On 2014/04/30 20:21:39, nweiz wrote:
> This shouldn't be public.

Done.

https://codereview.chromium.org/256753004/diff/120001/pkg/shelf/lib/src/messa...
pkg/shelf/lib/src/message.dart:186: final bool ignoreKeyCase;
On 2014/04/30 20:21:39, nweiz wrote:
> Once [ignoreKeyCase] is no longer public, there's no reason for this to have
the
> property at all.

Done.

[nweiz, 2014-05-05 20:05:54]

lgtm

https://codereview.chromium.org/256753004/diff/140001/pkg/shelf/lib/src/shelf...
File pkg/shelf/lib/src/shelf_unmodifiable_map.dart (right):

https://codereview.chromium.org/256753004/diff/140001/pkg/shelf/lib/src/shelf...
pkg/shelf/lib/src/shelf_unmodifiable_map.dart:31: return new
_EmptyShelfUnmodifiableMap<V>();
"new" -> "const"

https://codereview.chromium.org/256753004/diff/140001/pkg/shelf/lib/src/shelf...
pkg/shelf/lib/src/shelf_unmodifiable_map.dart:57: 
Unnecessary newline.

[kevmoo, 2014-05-06 20:25:57]

https://codereview.chromium.org/256753004/diff/140001/pkg/shelf/lib/src/shelf...
File pkg/shelf/lib/src/shelf_unmodifiable_map.dart (right):

https://codereview.chromium.org/256753004/diff/140001/pkg/shelf/lib/src/shelf...
pkg/shelf/lib/src/shelf_unmodifiable_map.dart:31: return new
_EmptyShelfUnmodifiableMap<V>();
On 2014/05/05 20:05:55, nweiz wrote:
> "new" -> "const"

Done.

https://codereview.chromium.org/256753004/diff/140001/pkg/shelf/lib/src/shelf...
pkg/shelf/lib/src/shelf_unmodifiable_map.dart:57: 
On 2014/05/05 20:05:55, nweiz wrote:
> Unnecessary newline.

Done.

[kevmoo, 2014-05-06 20:26:23]

Committed patchset #10 manually as r35825 (presubmit successful).