pkg/shelf: change helper method on Request and Response

pkg/shelf/lib/src/request.dart

Index: pkg/shelf/lib/src/request.dart
diff --git a/pkg/shelf/lib/src/request.dart b/pkg/shelf/lib/src/request.dart
index f7e011dc42609321c775d6d8ecc9bc7af582e78a..4a719601efbe07a2f91d9f18e7d55bee221f09f8 100644
--- a/pkg/shelf/lib/src/request.dart
+++ b/pkg/shelf/lib/src/request.dart
@@ -81,7 +81,9 @@ class Request extends Message {
             headers: headers, context: context) {
     if (method.isEmpty) throw new ArgumentError('method cannot be empty.');
 
-    // TODO(kevmoo) use isAbsolute property on Uri once Issue 18053 is fixed
+    // NOTE: A Uri with a #fragment is is not considered 'absolute' per
+    // http://tools.ietf.org/html/rfc3986#section-4.3
+    // So instead of using Uri.isAbsolute, just checking for an empty scheme

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

Use full sentences in comments.

[kevmoo, 2014/04/29 10:41:54]

Done.

     if (requestedUri.scheme.isEmpty) {

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

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]

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

[kevmoo, 2014/04/29 10:41:54]

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

       throw new ArgumentError('requstedUri must be an absolute URI.');
     }
@@ -107,6 +109,33 @@ class Request extends Message {
       throw new ArgumentError('scriptName and url cannot both be empty.');
     }
   }
+
+  /// Copies the request updated [context] and [headers] values.

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

Fix this sentence.

[kevmoo, 2014/04/29 10:41:54]

Done.

+  ///
+  /// New key-value pairs in [context] and [headers] will be added to the copied
+  /// [Request].
+  ///
+  /// If [context] or [headers] includes a key that already exists, the
+  /// key-value pair will replace the corresponding entry in the copied
+  /// [Request].
+  ///
+  /// All other context and header values from the [Request] will be included
+  /// in the copied [Request] unchanged.
+  Request copy({Map<String, String> headers, Map<String, Object> context}) {

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

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.

[kevmoo, 2014/04/29 10:41:54]

Done.

+    headers = _updatedMap(this.headers, headers);
+    context = _updatedMap(this.context, context);
+
+    return new Request(this.method, this.requestedUri,
+        protocolVersion: this.protocolVersion, headers: headers, url: this.url,
+        scriptName: this.scriptName, body: this.read(), context: context);

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

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.

[kevmoo, 2014/04/29 10:41:54]

Done.

+  }
+}
+
+Map _updatedMap(Map original, Map updates) {

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

Functions should have verb names, e.g. "_updateMap". Also, document this.

[kevmoo, 2014/04/29 10:41:54]

Done.

+  if (updates == null || updates.isEmpty) return original;
+
+  return new Map.from(original)
+      ..addAll(updates);
 }
 
 /// Computes `url` from the provided [Request] constructor arguments.