pkg/shelf: added `url` and `scriptName` named params to Request.change

pkg/shelf: added `url` and `scriptName` named params to Request.change

R=nweiz@google.com

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

[nweiz, 2014-05-22 19:50:42]

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

https://codereview.chromium.org/294123011/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:177: /// [Request.url] or an [ArgumentError] is
thrown.
This paragraph is confusing. I suggest: "If [scriptName] is provided and [url]
is not, [scriptName] must be a prefix of [this.url]. [url] will default to
[this.url] with this prefix removed."

It would also be nice to explain the purpose of this. Maybe "This allows routing
middleware to easily mount handlers under specific URLs."

https://codereview.chromium.org/294123011/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:190: ' match the beginning of the existing url
path.');
"match the beginning" -> "be a prefix"

https://codereview.chromium.org/294123011/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:195: 
Nit: I'd remove this newline.

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

https://codereview.chromium.org/294123011/diff/20001/pkg/shelf/test/request_t...
pkg/shelf/test/request_test.dart:173: test('updates url if striptName matches
existing url', () {
"striptName" -> "scriptName"

https://codereview.chromium.org/294123011/diff/20001/pkg/shelf/test/request_t...
pkg/shelf/test/request_test.dart:175: 
Nit: remove this newline and the next one. Same with other newlines between
assignments.

[kevmoo, 2014-05-22 20:38:35]

PTAL

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

https://codereview.chromium.org/294123011/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:177: /// [Request.url] or an [ArgumentError] is
thrown.
On 2014/05/22 19:50:42, nweiz wrote:
> This paragraph is confusing. I suggest: "If [scriptName] is provided and [url]
> is not, [scriptName] must be a prefix of [this.url]. [url] will default to
> [this.url] with this prefix removed."
> 
> It would also be nice to explain the purpose of this. Maybe "This allows
routing
> middleware to easily mount handlers under specific URLs."

Done.

https://codereview.chromium.org/294123011/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:190: ' match the beginning of the existing url
path.');
On 2014/05/22 19:50:42, nweiz wrote:
> "match the beginning" -> "be a prefix"

Done.

https://codereview.chromium.org/294123011/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:195: 
On 2014/05/22 19:50:42, nweiz wrote:
> Nit: I'd remove this newline.

Done.

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

https://codereview.chromium.org/294123011/diff/20001/pkg/shelf/test/request_t...
pkg/shelf/test/request_test.dart:173: test('updates url if striptName matches
existing url', () {
On 2014/05/22 19:50:42, nweiz wrote:
> "striptName" -> "scriptName"

Done.

[nweiz, 2014-05-23 21:43:33]

lgtm

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

https://codereview.chromium.org/294123011/diff/20001/pkg/shelf/lib/src/reques...
pkg/shelf/lib/src/request.dart:177: /// [Request.url] or an [ArgumentError] is
thrown.
On 2014/05/22 20:38:36, kevmoo wrote:
> On 2014/05/22 19:50:42, nweiz wrote:
> > This paragraph is confusing. I suggest: "If [scriptName] is provided and
[url]
> > is not, [scriptName] must be a prefix of [this.url]. [url] will default to
> > [this.url] with this prefix removed."
> > 
> > It would also be nice to explain the purpose of this. Maybe "This allows
> routing
> > middleware to easily mount handlers under specific URLs."
> 
> Done.

You didn't include any explanation of the purpose. Was that intentional?

[kevmoo, 2014-05-24 00:32:43]

Committed patchset #4 manually as r36601 (presubmit successful).