Add toUri and fromUri functions to pathos.

Add toUri and fromUri functions to pathos.

This also replaces the ubiqutous pathToFileUri/fileUriToPath functions.

R=rnystrom@google.com

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

[Bob Nystrom, 2013-06-13 00:21:18]

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

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:881: /// [path] is guaranteed to be absolute. Relative
paths are handled
"guaranteed" is confusing here. Is it required to be, or does it mean
pathToUri() will handle it correctly if it isn't?

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:891: : super._('posix', '/', '/', r'[^/]$', '/');
Since you are subclassing the styles now, let's make these abstract getters in
Style, instead of fields. Then override them instead of passing them to the
superclass constructor.

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:894: if (uri.scheme == '' || uri.scheme == 'file')
return uri.path;
What about URL decoding?

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:898: Uri pathToUri(String path) =>
Uri.parse('file://$path');
new Uri(scheme: "file", path: path);

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:917: return uri.path.replaceAll("/", "\\");
Doing this replaceAll for both arms seems redundant. How about:

var path = uri.path;
if (path.startsWith('/')) {
  ...
  path = path.replaceFirst(...)
}
return path.replaceAll(...)

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:928: return Uri.parse('file:${path.replaceAll("\\",
"/")}');
Use the regular Uri constructor here instead of making a string just to the
parse it. Below too.

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/test/pathos_posix_...
File pkg/pathos/test/pathos_posix_test.dart (right):

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/test/pathos_posix_...
pkg/pathos/test/pathos_posix_test.dart:416: expect(builder.toUri('foo/bar'),
Uri.parse('foo/bar'));
This feels odd to me. Will users expect this, or should we convert "foo/bar" to
an absolute path and then return a "file:" URL?

[Søren Gjesse, 2013-06-13 07:13:32]

dbc

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

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:898: Uri pathToUri(String path) =>
Uri.parse('file://$path');
On 2013/06/13 00:21:18, Bob Nystrom wrote:
> new Uri(scheme: "file", path: path);

The path should also be encoded

new Uri(scheme: "file", path: Uri.encodeComponent(path))

[nweiz, 2013-06-19 00:50:27]

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

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:881: /// [path] is guaranteed to be absolute. Relative
paths are handled
On 2013/06/13 00:21:18, Bob Nystrom wrote:
> "guaranteed" is confusing here. Is it required to be, or does it mean
> pathToUri() will handle it correctly if it isn't?

This documentation is intended for implementors of [Style]. It's a guarantee
that pathos will only pass absolute paths to [pathToUri].

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:891: : super._('posix', '/', '/', r'[^/]$', '/');
On 2013/06/13 00:21:18, Bob Nystrom wrote:
> Since you are subclassing the styles now, let's make these abstract getters in
> Style, instead of fields. Then override them instead of passing them to the
> superclass constructor.

Done.

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:894: if (uri.scheme == '' || uri.scheme == 'file')
return uri.path;
On 2013/06/13 00:21:18, Bob Nystrom wrote:
> What about URL decoding?

Done.

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:898: Uri pathToUri(String path) =>
Uri.parse('file://$path');
On 2013/06/13 07:13:32, Søren Gjesse wrote:
> On 2013/06/13 00:21:18, Bob Nystrom wrote:
> > new Uri(scheme: "file", path: path);
> 
> The path should also be encoded
> 
> new Uri(scheme: "file", path: Uri.encodeComponent(path))

Done.

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:917: return uri.path.replaceAll("/", "\\");
On 2013/06/13 00:21:18, Bob Nystrom wrote:
> Doing this replaceAll for both arms seems redundant. How about:
> 
> var path = uri.path;
> if (path.startsWith('/')) {
>   ...
>   path = path.replaceFirst(...)
> }
> return path.replaceAll(...)

Done.

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:928: return Uri.parse('file:${path.replaceAll("\\",
"/")}');
On 2013/06/13 00:21:18, Bob Nystrom wrote:
> Use the regular Uri constructor here instead of making a string just to the
> parse it. Below too.

Done.

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/test/pathos_posix_...
File pkg/pathos/test/pathos_posix_test.dart (right):

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/test/pathos_posix_...
pkg/pathos/test/pathos_posix_test.dart:416: expect(builder.toUri('foo/bar'),
Uri.parse('foo/bar'));
On 2013/06/13 00:21:18, Bob Nystrom wrote:
> This feels odd to me. Will users expect this, or should we convert "foo/bar"
to
> an absolute path and then return a "file:" URL?

I think this is correct. Think about it from the perspective of manipulating
paths and imported URIs. If you know the relative path from file A to file B and
you want to convert that to an importable URI, you definitely want it to stay
relative during that conversion.

[Bob Nystrom, 2013-06-19 18:02:18]

One nit then LGTM.

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

https://codereview.chromium.org/16848002/diff/1/pkg/pathos/lib/path.dart#newc...
pkg/pathos/lib/path.dart:881: /// [path] is guaranteed to be absolute. Relative
paths are handled
On 2013/06/19 00:50:27, nweiz wrote:
> On 2013/06/13 00:21:18, Bob Nystrom wrote:
> > "guaranteed" is confusing here. Is it required to be, or does it mean
> > pathToUri() will handle it correctly if it isn't?
> 
> This documentation is intended for implementors of [Style]. It's a guarantee
> that pathos will only pass absolute paths to [pathToUri].

Ah, that makes sense. It isn't strictly true that pathos is the only thing that
will ever call this, though, so can you reword the docs to make it clear that
this is a requirement for the argument?

[nweiz, 2013-06-19 19:06:36]

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