Support a URL style for pathos.

Support a URL style for pathos.

BUG=11131
Committed: https://code.google.com/p/dart/source/detail?r=23758

[Bob Nystrom, 2013-06-06 22:50:40]

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart
File pkg/pathos/lib/path.dart (right):

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:364: /// relative to the root of the current URL. Since
root-relative paths are still
Long lines.

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:378: /// relative to the root of the current URL. Since
root-relative paths are still
Ditto.

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:427: var oldRoot = this.rootPrefix(buffer.toString());
A couple of things here:

1. Calling toString() on the buffer multiple times for each part feels slow. How
about keeping track of whether or not the current buffer is absolute or root
relative? You can make two bools that default to false and then set them as
appropriate in the if (this.isAbsolute(part)) arm.

2. It isn't obvious what this is for. How about a "if the new part is root
relative, it replaces the previous path but preserves the rootPrefix". 

3. Is that actually the correct behavior? Do we want to support a base URL or
something?

4. I don't understand what the !this.isRootRelative(buffer.toString()) case is
for.

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:428: buffer = new StringBuffer();
buffer.clear() seems cleaner to me. Below too?

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:690: /// a separator is needed, especially for root
path components.
Can you explain what these rules are somewhere?

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:710: return match[0];
Seems weird to refer back to match here after delegating to getRelativeRoot. How
about flipping the if to be "match != null"?

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:746: /// The path separators preceeding each part.
"preceeding" -> "preceding".

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/test/pathos_url_te...
File pkg/pathos/test/pathos_url_test.dart (right):

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/test/pathos_url_te...
pkg/pathos/test/pathos_url_test.dart:345:
expect(builder.normalize('http://dartlang.org/..'), 'http://dartlang.org');
Long line.

[nweiz, 2013-06-07 01:04:30]

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart
File pkg/pathos/lib/path.dart (right):

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:364: /// relative to the root of the current URL. Since
root-relative paths are still
On 2013/06/06 22:50:40, Bob Nystrom wrote:
> Long lines.

Done.

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:378: /// relative to the root of the current URL. Since
root-relative paths are still
On 2013/06/06 22:50:40, Bob Nystrom wrote:
> Ditto.

Done.

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:427: var oldRoot = this.rootPrefix(buffer.toString());
On 2013/06/06 22:50:40, Bob Nystrom wrote:
> A couple of things here:
> 
> 1. Calling toString() on the buffer multiple times for each part feels slow.
How
> about keeping track of whether or not the current buffer is absolute or root
> relative? You can make two bools that default to false and then set them as
> appropriate in the if (this.isAbsolute(part)) arm.

Done.

> 2. It isn't obvious what this is for. How about a "if the new part is root
> relative, it replaces the previous path but preserves the rootPrefix". 

Done.

> 3. Is that actually the correct behavior? Do we want to support a base URL or
> something?

Yes. This makes pathos.join("http://foo.com", "/bar") return
"http://foo.com/bar", which is correct. It's analogous to relative parts being
interpreted as relative to the prior components of the path.

> 4. I don't understand what the !this.isRootRelative(buffer.toString()) case is
> for.

If the existing path is also root-relative, we don't want to preserve anything
about it. For example, path.join("/foo", "/bar") should just return "/bar".

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:428: buffer = new StringBuffer();
On 2013/06/06 22:50:40, Bob Nystrom wrote:
> buffer.clear() seems cleaner to me. Below too?

Done.

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:690: /// a separator is needed, especially for root
path components.
On 2013/06/06 22:50:40, Bob Nystrom wrote:
> Can you explain what these rules are somewhere?

Done.

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:710: return match[0];
On 2013/06/06 22:50:40, Bob Nystrom wrote:
> Seems weird to refer back to match here after delegating to getRelativeRoot.
How
> about flipping the if to be "match != null"?

Done.

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:746: /// The path separators preceeding each part.
On 2013/06/06 22:50:40, Bob Nystrom wrote:
> "preceeding" -> "preceding".

Done.

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/test/pathos_url_te...
File pkg/pathos/test/pathos_url_test.dart (right):

https://codereview.chromium.org/16580005/diff/1/pkg/pathos/test/pathos_url_te...
pkg/pathos/test/pathos_url_test.dart:345:
expect(builder.normalize('http://dartlang.org/..'), 'http://dartlang.org');
On 2013/06/06 22:50:40, Bob Nystrom wrote:
> Long line.

Done.

[Bob Nystrom, 2013-06-07 15:47:34]

LTGM, very nice!

[nweiz, 2013-06-07 18:30:01]

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