Handle special characters in path<->uri conversion.

Handle special characters in path<->uri conversion.

Also cleaned up some docs a bit.

R=nweiz@google.com

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

[nweiz, 2013-07-25 20:47:01]

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

https://codereview.chromium.org/20130004/diff/1/pkg/path/lib/path.dart#newcode35
pkg/path/lib/path.dart:35: /// This calls the top-level [join()] function to
join "directory" and
Nit: "[join()]" -> "[join]".

https://codereview.chromium.org/20130004/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:794: var parsed = _parse(path);
Why use [_parse] here instead of [split]?

https://codereview.chromium.org/20130004/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:916: Uri relativePathToUri(Iterable<String> parts) => new
Uri(pathSegments: parts);
It's weird that this takes a list of parts but [absolutePathToUri] takes a
string. It feels like this should take a string and split it apart in the
default implementation. Then the URL implementation can just be
"Uri.parse(path)".

https://codereview.chromium.org/20130004/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:1047: return
super.relativePathToUri(parts.map(Uri.decodeComponent));
Seems unnecessary to parse and then re-serialize the URI. See above comment.

[Bob Nystrom, 2013-07-25 23:20:10]

Thanks!

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

https://codereview.chromium.org/20130004/diff/1/pkg/path/lib/path.dart#newcode35
pkg/path/lib/path.dart:35: /// This calls the top-level [join()] function to
join "directory" and
On 2013/07/25 20:47:02, nweiz wrote:
> Nit: "[join()]" -> "[join]".

Done.

https://codereview.chromium.org/20130004/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:794: var parsed = _parse(path);
On 2013/07/25 20:47:02, nweiz wrote:
> Why use [_parse] here instead of [split]?

This path is known to be relative, so I don't think split() buys much. It calls
_parse() itself, so this seemed a bit more straightforward.

https://codereview.chromium.org/20130004/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:916: Uri relativePathToUri(Iterable<String> parts) => new
Uri(pathSegments: parts);
On 2013/07/25 20:47:02, nweiz wrote:
> It's weird that this takes a list of parts but [absolutePathToUri] takes a
> string. It feels like this should take a string and split it apart in the
> default implementation. Then the URL implementation can just be
> "Uri.parse(path)".

Done. Good call.

https://codereview.chromium.org/20130004/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:1047: return
super.relativePathToUri(parts.map(Uri.decodeComponent));
On 2013/07/25 20:47:02, nweiz wrote:
> Seems unnecessary to parse and then re-serialize the URI. See above comment.

Done.

[nweiz, 2013-07-25 23:44:27]

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

https://codereview.chromium.org/20130004/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:794: var parsed = _parse(path);
On 2013/07/25 23:20:10, Bob Nystrom wrote:
> On 2013/07/25 20:47:02, nweiz wrote:
> > Why use [_parse] here instead of [split]?
> 
> This path is known to be relative, so I don't think split() buys much. It
calls
> _parse() itself, so this seemed a bit more straightforward.

It's not more straightforward to a reader of the code who's more familiar with
the external API than the internal one (that is, everyone but you and maybe me).

In general, I think it's almost always cleaner to implement stuff in terms of
the public API if possible.

https://codereview.chromium.org/20130004/diff/5001/pkg/path/lib/path.dart
File pkg/path/lib/path.dart (right):

https://codereview.chromium.org/20130004/diff/5001/pkg/path/lib/path.dart#new...
pkg/path/lib/path.dart:916: new Uri(pathSegments: path.split(separatorPattern));
I'm worried about edge cases from using [String.split] here instead of
[Builder.split]. The posix and windows styles both already keep a builder
around; maybe it's time to move that into the Style superclass. Something like

    Builder get builder => new Builder(style: this);

If not, at least test the same edge cases for converting from paths to URIs that
get tested for [split].

[Bob Nystrom, 2013-07-25 23:58:37]

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

https://codereview.chromium.org/20130004/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:794: var parsed = _parse(path);
On 2013/07/25 23:44:27, nweiz wrote:
> On 2013/07/25 23:20:10, Bob Nystrom wrote:
> > On 2013/07/25 20:47:02, nweiz wrote:
> > > Why use [_parse] here instead of [split]?
> > 
> > This path is known to be relative, so I don't think split() buys much. It
> calls
> > _parse() itself, so this seemed a bit more straightforward.
> 
> It's not more straightforward to a reader of the code who's more familiar with
> the external API than the internal one (that is, everyone but you and maybe
me).
> 
> In general, I think it's almost always cleaner to implement stuff in terms of
> the public API if possible.

This code was removed anyway, so it's a moot point. :)

https://codereview.chromium.org/20130004/diff/5001/pkg/path/lib/path.dart
File pkg/path/lib/path.dart (right):

https://codereview.chromium.org/20130004/diff/5001/pkg/path/lib/path.dart#new...
pkg/path/lib/path.dart:916: new Uri(pathSegments: path.split(separatorPattern));
On 2013/07/25 23:44:27, nweiz wrote:
> I'm worried about edge cases from using [String.split] here instead of
> [Builder.split]. The posix and windows styles both already keep a builder
> around; maybe it's time to move that into the Style superclass. Something like
> 
>     Builder get builder => new Builder(style: this);
> 
> If not, at least test the same edge cases for converting from paths to URIs
that
> get tested for [split].

Done.

[nweiz, 2013-07-26 00:08:27]

lgtm++

[Bob Nystrom, 2013-07-26 00:15:47]

Committed patchset #3 manually as r25512 (presubmit successful).