pkg/shelf: change helper method on Request and Response

pkg/shelf/lib/src/message.dart

Index: pkg/shelf/lib/src/message.dart
diff --git a/pkg/shelf/lib/src/message.dart b/pkg/shelf/lib/src/message.dart
index 1b2e966c4660b3375519c985a459362257e00408..ef035cb0af5413b1d8e57a3fd32ba9428c5aa50c 100644
--- a/pkg/shelf/lib/src/message.dart
+++ b/pkg/shelf/lib/src/message.dart
@@ -43,9 +43,8 @@ abstract class Message {
   /// If [headers] is `null`, it is treated as empty.
   Message(this._body, {Map<String, String> headers,
         Map<String, Object> context})
-      : this.headers = _getIgnoreCaseMapView(headers),
-        this.context = new pc.UnmodifiableMapView(
-            context == null ? const {} : new Map.from(context));
+      : this.headers = new _ShelfUnmodifiableMap<String>(headers, true),
+        this.context = new _ShelfUnmodifiableMap<Object>(context, false);
 
   /// The contents of the content-length field in [headers].
   ///
@@ -112,18 +111,68 @@ abstract class Message {
     if (encoding == null) encoding = UTF8;
     return Chain.track(encoding.decodeStream(read()));
   }
+
+  /// Creates a new [Message] by copying existing values and applying specified
+  /// changes.
+  Message change({Map<String, String> headers, Map<String, Object> context});
 }
 
-/// Creates on an unmodifiable map of [headers] with case-insensitive access.
-Map<String, String> _getIgnoreCaseMapView(Map<String, String> headers) {
-  if (headers == null) return const {};
-  // TODO(kevmoo) generalize this model with a 'canonical map' to align with
-  // similiar implementation in http pkg [BaseRequest].
-  var map = new LinkedHashMap<String, String>(
-      equals: (key1, key2) => key1.toLowerCase() == key2.toLowerCase(),
-      hashCode: (key) => key.toLowerCase().hashCode);
+/// Returns a [Map] with the values from [original] and the values from
+/// [updates].
+///
+/// For keys that are the same between [original] and [updates], the value in
+/// [updates] is used.
+///
+/// If [updates] is `null` or empty, [original] is returned unchanged.
+Map updateMap(Map original, Map updates) {

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

Put this in util.

[kevmoo, 2014/04/30 14:27:52]

Neither Request nor Response import util at the moment.

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

[nweiz, 2014/04/30 20:21:39]

It's not onerous to add an import statement.
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.

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

*grumble* Done *eye roll*

+  if (updates == null || updates.isEmpty) return original;
+
+  return new Map.from(original)
+      ..addAll(updates);
+}
 
-  map.addAll(headers);
+/// A simple wrapper over [pc.UnmodifiableMapView] which avoids re-wrapping
+/// itself.
+///
+/// If [source] is a [_ShelfUnmodifiableMap] with matcher [ignoreKeyCase], then

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

"matcher" -> "matching"

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

[kevmoo, 2014/04/30 14:27:52]

Done.

+/// [source] is returned.
+///
+/// If [source] is `null` it is treated like an empty map.
+///
+/// If [ignoreKeyCase] is `true`, the keys will have case-insensitive access.
+///
+/// [source] is copied to a new [Map] to ensure changes to the paramater value
+/// after constructions are not reflected.
+class _ShelfUnmodifiableMap<V> extends pc.UnmodifiableMapView<String, V> {

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

This should probably go in its own library.

[kevmoo, 2014/04/30 14:27:52]

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.

[nweiz, 2014/04/30 20:21:39]

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

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

fine fine Done yeah yeah

+  final bool ignoreKeyCase;

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

Document this field.

[kevmoo, 2014/04/30 14:27:52]

Done.

+
+  factory _ShelfUnmodifiableMap(Map<String, V> source, bool ignoreKeyCase) {

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

Make [ignoreKeyCase] a named argument so it's clearer at the call site.

[kevmoo, 2014/04/30 14:27:52]

Done.

+    if (source is _ShelfUnmodifiableMap<V> &&
+        source.ignoreKeyCase == ignoreKeyCase) {
+      return source;
+    }
+
+    if (source == null || source.isEmpty) {
+      return new _ShelfUnmodifiableMap<V>._(const {}, ignoreKeyCase);

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

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});

[kevmoo, 2014/04/30 14:27:52]

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

+    }
+
+    if (ignoreKeyCase) {
+      // TODO(kevmoo) generalize this model with a 'canonical map' to align with
+      // similiar implementation in http pkg [BaseRequest].
+      var map = new LinkedHashMap<String, V>(
+          equals: (key1, key2) => key1.toLowerCase() == key2.toLowerCase(),
+          hashCode: (key) => key.toLowerCase().hashCode);
+
+      map.addAll(source);
+
+      source = map;
+    } else {
+      source = new Map<String, V>.from(source);
+    }
+
+    return new _ShelfUnmodifiableMap<V>._(source, ignoreKeyCase);
+  }
 
-  return new pc.UnmodifiableMapView<String, String>(map);
+  _ShelfUnmodifiableMap._(Map<String, V> source, this.ignoreKeyCase)
+      : super(source);
 }