Add Bob's path library to pub.

Add Bob's path library to pub.

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

[nweiz, 2012-12-08 01:46:31]

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart
File utils/pub/path.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode27
utils/pub/path.dart:27: /// Gets the file extension of [path]; the portion after
the last `.` in the
nit: ";" -> ":".

Also this should probably be "after and including", since it does return the
".".

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode37
utils/pub/path.dart:37: ///     path.extension('~/.bashrc'); // -> ''
Maybe give an example of a dotfile with an extension as well? Also a
double-extension file?

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode40
utils/pub/path.dart:40: /// Gets the part of [path] after the last separator on
the current platform.
"on the current platform" is implicit. Also below.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode44
utils/pub/path.dart:44: String filename(String path) => _builder.filename(path);
This should be called "basename", for consistency with Unix and because it's not
always a file.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode54
utils/pub/path.dart:54: /// relative path. On Mac and Unix systems, relative
paths start with a `/`
"relative paths" -> "absolute paths". Also, OS X is a Unix system. Same below.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode78
utils/pub/path.dart:78: ///
Nit: extra line

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode98
utils/pub/path.dart:98: /// Converts [path] to an equivalent relative path from
the current directory.
This doesn't always return a relative path. On Windows, there is no relative
path to "D:\" from "C:\". The documentation should mention this edge case.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode112
utils/pub/path.dart:112: class Builder {
Any comments above obviously apply to the parallel instance methods as well.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode113
utils/pub/path.dart:113: /// Creates a new path builder for the given style and
root directory.
It may be worth emphasizing more that root is always assumed to be a directory.
Resolving paths relative to files work differently than resolving them relative
to directories, so if people don't understand this it may lead to considerable
confusion.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode116
utils/pub/path.dart:116: /// [root] is omitted, it defaults to the current
working directory.
Mention that if `root` is relative, it's considered relative to the current
working directory.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode126
utils/pub/path.dart:126: if (root == null) root = new
io.Directory.current().path;
Nit: it'd be a little cleaner to use `current` here.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode241
utils/pub/path.dart:241: if (path == '') return path;
An empty string is never a valid path. Why is there special handling for it
here?

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode252
utils/pub/path.dart:252: ///     builder.join('path', 'to', 'foo'); // ->
'root/path/to/foo'
".join" -> ".resolve"

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode264
utils/pub/path.dart:264: /// Converts [path] to an equivalent relative path
starting at [root].
"an equivalent path relative to [root]"

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode275
utils/pub/path.dart:275: // If the given path is relative, resolve it relative
to the base.
This seems unnecessary. If it's already relative to the root, why not just
normalize and return it?

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode288
utils/pub/path.dart:288: if (baseParsed.parts[0] != pathParsed.parts[0]) break;
while (baseParsed.parts.length > 0 && pathParsed.parts.length > 0 &&
       baseParsed.parts[0] == pathParsed.parts[0]) {

would be slightly cleaner.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode314
utils/pub/path.dart:314: String withoutExtension(String path) {
Why isn't this using _parse?

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode315
utils/pub/path.dart:315: var lastSeparator = path.lastIndexOf(separator);
Isn't this dangerous with windows paths that use `/` instead of `\`?

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode338
utils/pub/path.dart:338: separators.add(match[0]);
Why is it useful to keep track of separators? It just seems to make the
manipulation code more complicated.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode375
utils/pub/path.dart:375: /// ("\\") for UNC paths.
UNC prefixes should encompass the server name as well (e.g. `\\server\`).

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode379
utils/pub/path.dart:379: Style._(this.name, this.separator, String
separatorPattern, String rootPattern)
Long line

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode381
utils/pub/path.dart:381: _rootPattern = new RegExp('^$rootPattern');
This RegExp should be '^($rootPattern)'.

[Bob Nystrom, 2012-12-08 03:36:53]

Some nits, otherwise LGTM.

https://codereview.chromium.org/11475046/diff/1/utils/pub/command_lish.dart
File utils/pub/command_lish.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:16: import 'path.dart' as path;
Do you need path here? I don't see any uses of it in the diff.

https://codereview.chromium.org/11475046/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/pub/io.dart#newcode972
utils/pub/io.dart:972: /// remaining path. Takes into account Windows' quirky
absolute paths syntaxes.
Add a TODO to move this (or something similar) into path.

https://codereview.chromium.org/11475046/diff/1/utils/pub/package.dart
File utils/pub/package.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/pub/package.dart#newcode8
utils/pub/package.dart:8: import 'path.dart' as path;
Is this being used here?

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_pos...
File utils/tests/pub/path/path_posix_test.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_pos...
utils/tests/pub/path/path_posix_test.dart:246: expect(builder.resolve('a', 'b',
'c', 'd', 'e', 'f', 'g'), '/root/path/a/b/c/d/e/f/g');
Long lines.

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_win...
File utils/tests/pub/path/path_windows_test.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_win...
utils/tests/pub/path/path_windows_test.dart:230: expect(r.relative(r'C:\a\b'),
b.join(b.relative(r'C:\'), r'..\..\a\b'));
Long line.

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_win...
utils/tests/pub/path/path_windows_test.dart:261: expect(builder.resolve('a',
'b', 'c', 'd', 'e', 'f', 'g'), r'C:\root\path\a\b\c\d\e\f\g');
Long lines.

[nweiz, 2012-12-08 03:43:45]

https://codereview.chromium.org/11475046/diff/1/utils/pub/command_lish.dart
File utils/pub/command_lish.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:16: import 'path.dart' as path;
On 2012/12/08 03:36:53, Bob Nystrom wrote:
> Do you need path here? I don't see any uses of it in the diff.

Good catch. I probably changed something to use path and then back to io.dart
for the entry resolution.

https://codereview.chromium.org/11475046/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/pub/io.dart#newcode972
utils/pub/io.dart:972: /// remaining path. Takes into account Windows' quirky
absolute paths syntaxes.
On 2012/12/08 03:36:53, Bob Nystrom wrote:
> Add a TODO to move this (or something similar) into path.

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/package.dart
File utils/pub/package.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/pub/package.dart#newcode8
utils/pub/package.dart:8: import 'path.dart' as path;
On 2012/12/08 03:36:53, Bob Nystrom wrote:
> Is this being used here?

Nope, removed.

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_pos...
File utils/tests/pub/path/path_posix_test.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_pos...
utils/tests/pub/path/path_posix_test.dart:246: expect(builder.resolve('a', 'b',
'c', 'd', 'e', 'f', 'g'), '/root/path/a/b/c/d/e/f/g');
On 2012/12/08 03:36:53, Bob Nystrom wrote:
> Long lines.

Done.

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_tes...
File utils/tests/pub/path/path_test.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_tes...
utils/tests/pub/path/path_test.dart:15: expect(path.Style.posix.name, 'posix');
The style elsewhere in Dart is to explicitly use equals() rather than relying on
the implicit behavior. I kind of like that, since it makes it read like a
sentence.

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_win...
File utils/tests/pub/path/path_windows_test.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_win...
utils/tests/pub/path/path_windows_test.dart:16: if (new path.Builder().style ==
path.Style.windows) {
Why is it that you only have one test that checks whether the top-level
functions use the correct path style? It seems that if it's worth testing any of
them, it's worth testing all of them.

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_win...
utils/tests/pub/path/path_windows_test.dart:28: test('extension', () {
Test dotfiles with extensions here and in filenameWithoutExtension.

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_win...
utils/tests/pub/path/path_windows_test.dart:64: expect(builder.isAbsolute(''),
false);
isFalse/isTrue

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_win...
utils/tests/pub/path/path_windows_test.dart:230: expect(r.relative(r'C:\a\b'),
b.join(b.relative(r'C:\'), r'..\..\a\b'));
On 2012/12/08 03:36:53, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/11475046/diff/1/utils/tests/pub/path/path_win...
utils/tests/pub/path/path_windows_test.dart:261: expect(builder.resolve('a',
'b', 'c', 'd', 'e', 'f', 'g'), r'C:\root\path\a\b\c\d\e\f\g');
On 2012/12/08 03:36:53, Bob Nystrom wrote:
> Long lines.

Done.

[Bob Nystrom, 2012-12-08 05:12:33]

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart
File utils/pub/path.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode27
utils/pub/path.dart:27: /// Gets the file extension of [path]; the portion after
the last `.` in the
On 2012/12/08 01:46:31, nweiz wrote:
> nit: ";" -> ":".
> 
> Also this should probably be "after and including", since it does return the
> ".".

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode37
utils/pub/path.dart:37: ///     path.extension('~/.bashrc'); // -> ''
On 2012/12/08 01:46:31, nweiz wrote:
> Maybe give an example of a dotfile with an extension as well?

Done.

> Also a double-extension file?

Double-extension is covered above.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode40
utils/pub/path.dart:40: /// Gets the part of [path] after the last separator on
the current platform.
On 2012/12/08 01:46:31, nweiz wrote:
> "on the current platform" is implicit. Also below.

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode44
utils/pub/path.dart:44: String filename(String path) => _builder.filename(path);
On 2012/12/08 01:46:31, nweiz wrote:
> This should be called "basename", for consistency with Unix and because it's
not
> always a file.

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode54
utils/pub/path.dart:54: /// relative path. On Mac and Unix systems, relative
paths start with a `/`
On 2012/12/08 01:46:31, nweiz wrote:
> "relative paths" -> "absolute paths". Also, OS X is a Unix system. Same below.

Done. Changed to "POSIX".

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode78
utils/pub/path.dart:78: ///
On 2012/12/08 01:46:31, nweiz wrote:
> Nit: extra line

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode98
utils/pub/path.dart:98: /// Converts [path] to an equivalent relative path from
the current directory.
On 2012/12/08 01:46:31, nweiz wrote:
> This doesn't always return a relative path. On Windows, there is no relative
> path to "D:\" from "C:\". The documentation should mention this edge case.

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode112
utils/pub/path.dart:112: class Builder {
On 2012/12/08 01:46:31, nweiz wrote:
> Any comments above obviously apply to the parallel instance methods as well.

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode113
utils/pub/path.dart:113: /// Creates a new path builder for the given style and
root directory.
On 2012/12/08 01:46:31, nweiz wrote:
> It may be worth emphasizing more that root is always assumed to be a
directory.
> Resolving paths relative to files work differently than resolving them
relative
> to directories, so if people don't understand this it may lead to considerable
> confusion.

I think this will be OK? I'm not sure how to emphasize it more.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode116
utils/pub/path.dart:116: /// [root] is omitted, it defaults to the current
working directory.
On 2012/12/08 01:46:31, nweiz wrote:
> Mention that if `root` is relative, it's considered relative to the current
> working directory.

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode126
utils/pub/path.dart:126: if (root == null) root = new
io.Directory.current().path;
On 2012/12/08 01:46:31, nweiz wrote:
> Nit: it'd be a little cleaner to use `current` here.

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode241
utils/pub/path.dart:241: if (path == '') return path;
On 2012/12/08 01:46:31, nweiz wrote:
> An empty string is never a valid path. Why is there special handling for it
> here?

It's following python. It feels a bit weird, but I think it might be marginally
helpful if you're normalizing a pile of strings and some of them happen to be
empty strings. In that case, I think yielding an empty string in return is
fairly innocuous.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode252
utils/pub/path.dart:252: ///     builder.join('path', 'to', 'foo'); // ->
'root/path/to/foo'
On 2012/12/08 01:46:31, nweiz wrote:
> ".join" -> ".resolve"

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode264
utils/pub/path.dart:264: /// Converts [path] to an equivalent relative path
starting at [root].
On 2012/12/08 01:46:31, nweiz wrote:
> "an equivalent path relative to [root]"

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode275
utils/pub/path.dart:275: // If the given path is relative, resolve it relative
to the base.
On 2012/12/08 01:46:31, nweiz wrote:
> This seems unnecessary. If it's already relative to the root, why not just
> normalize and return it?

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode288
utils/pub/path.dart:288: if (baseParsed.parts[0] != pathParsed.parts[0]) break;
On 2012/12/08 01:46:31, nweiz wrote:
> while (baseParsed.parts.length > 0 && pathParsed.parts.length > 0 &&
>        baseParsed.parts[0] == pathParsed.parts[0]) {
> 
> would be slightly cleaner.

Felt a bit gratuitous to me, but done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode314
utils/pub/path.dart:314: String withoutExtension(String path) {
On 2012/12/08 01:46:31, nweiz wrote:
> Why isn't this using _parse?

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode315
utils/pub/path.dart:315: var lastSeparator = path.lastIndexOf(separator);
On 2012/12/08 01:46:31, nweiz wrote:
> Isn't this dangerous with windows paths that use `/` instead of `\`?

Good catch. Added a test and made this use _parse. It was older code.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode338
utils/pub/path.dart:338: separators.add(match[0]);
On 2012/12/08 01:46:31, nweiz wrote:
> Why is it useful to keep track of separators? It just seems to make the
> manipulation code more complicated.

As much as possible, path retains the information you give it. It doesn't try to
clean up or normalize your path unless you ask it to. So, if you give it:

a/b\c/d.txt

and call path.withoutExtension(), I want you to get: a/b\c/d and not a\b\c\d

That means keeping track of which separators were used. Nit-picky, but I'm
trying to keep as much user-given information as I can.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode375
utils/pub/path.dart:375: /// ("\\") for UNC paths.
On 2012/12/08 01:46:31, nweiz wrote:
> UNC prefixes should encompass the server name as well (e.g. `\\server\`).

Added a TODO for now.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode379
utils/pub/path.dart:379: Style._(this.name, this.separator, String
separatorPattern, String rootPattern)
On 2012/12/08 01:46:31, nweiz wrote:
> Long line

Done.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode381
utils/pub/path.dart:381: _rootPattern = new RegExp('^$rootPattern');
On 2012/12/08 01:46:31, nweiz wrote:
> This RegExp should be '^($rootPattern)'.

How come?

[nweiz, 2012-12-10 21:30:59]

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart
File utils/pub/path.dart (right):

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode241
utils/pub/path.dart:241: if (path == '') return path;
On 2012/12/08 05:12:33, Bob Nystrom wrote:
> On 2012/12/08 01:46:31, nweiz wrote:
> > An empty string is never a valid path. Why is there special handling for it
> > here?
> 
> It's following python. It feels a bit weird, but I think it might be
marginally
> helpful if you're normalizing a pile of strings and some of them happen to be
> empty strings. In that case, I think yielding an empty string in return is
> fairly innocuous.

Even if it works, it's a little weird that you're explicitly checking for it
here and nowhere else. Why not just let it fall through the logic naturally?

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode338
utils/pub/path.dart:338: separators.add(match[0]);
On 2012/12/08 05:12:33, Bob Nystrom wrote:
> On 2012/12/08 01:46:31, nweiz wrote:
> > Why is it useful to keep track of separators? It just seems to make the
> > manipulation code more complicated.
> 
> As much as possible, path retains the information you give it. It doesn't try
to
> clean up or normalize your path unless you ask it to. So, if you give it:
> 
> a/b\c/d.txt
> 
> and call path.withoutExtension(), I want you to get: a/b\c/d and not a\b\c\d
> 
> That means keeping track of which separators were used. Nit-picky, but I'm
> trying to keep as much user-given information as I can.

I think this will end up causing more pain than it's worth down the line if
_parse ever becomes exposed. It's a nice idea to do no normalization, but in
this case I think it's worth it to make the manipulation easier, especially
since there's no semantic difference at all between the two forms.

https://codereview.chromium.org/11475046/diff/1/utils/pub/path.dart#newcode381
utils/pub/path.dart:381: _rootPattern = new RegExp('^$rootPattern');
On 2012/12/08 05:12:33, Bob Nystrom wrote:
> On 2012/12/08 01:46:31, nweiz wrote:
> > This RegExp should be '^($rootPattern)'.
> 
> How come?

Otherwise on Windows it ends up as r'^\\\\|[a-zA-Z]:[/\\]'; the
beginning-of-string character is only required for "\\", not for "C:\".