Handle relative paths where the trailing directory has an extension.

Handle relative paths where the trailing directory has an extension.

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

[nweiz, 2012-12-11 02:09:42]

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

https://codereview.chromium.org/11512011/diff/1/utils/pub/path.dart#newcode316
utils/pub/path.dart:316: if (parsed.hasTrailingSeparator) return
parsed.toString();
I really don't like changing behavior based on whether or not there's a trailing
separator. Most programs treat "foo/" the same as "foo", and those that don't
are inconsistent about what "foo/" means.

https://codereview.chromium.org/11512011/diff/1/utils/pub/path.dart#newcode320
utils/pub/path.dart:320: parsed.parts[parsed.parts.length - 1] = split[0];
Calling a private method here seems like a code smell. Why can't you do
`parsed.parts[parsed.parts.length - 1] = parsed.filenameWithoutExtension`
instead?

https://codereview.chromium.org/11512011/diff/1/utils/pub/path.dart#newcode506
utils/pub/path.dart:506: // If there is a dot (and it's not the first character,
like '.bashrc').
This looks like it's supposed to go a couple lines further down.

https://codereview.chromium.org/11512011/diff/1/utils/pub/path.dart#newcode509
utils/pub/path.dart:509: parts[parts.length - 1] = file.substring(0, lastDot);
Why is this useful? Doesn't this mean that calling `parsed.extension` or
`parsed.filenameWithoutExtension` will destructively modify `parsed`?

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

https://codereview.chromium.org/11512011/diff/1/utils/tests/pub/path/path_win...
utils/tests/pub/path/path_windows_test.dart:304:
expect(builder.withoutExtension(r'a.b/c'), r'a.b/c');
This should probably also be added to path_posix_test.

[Bob Nystrom, 2012-12-11 02:24:59]

Thanks!

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

https://codereview.chromium.org/11512011/diff/1/utils/pub/path.dart#newcode316
utils/pub/path.dart:316: if (parsed.hasTrailingSeparator) return
parsed.toString();
On 2012/12/11 02:09:42, nweiz wrote:
> I really don't like changing behavior based on whether or not there's a
trailing
> separator. Most programs treat "foo/" the same as "foo", and those that don't
> are inconsistent about what "foo/" means.

This is how Python works. I think it would be confusing if
withoutExtension("foo.bar/") returned "foo". I believe the expectation is that
it will strip the extension from the file name, if there is a file at the end of
the path. If you have a trailing separator, I don't think there's a file at the
end of the path.

https://codereview.chromium.org/11512011/diff/1/utils/pub/path.dart#newcode320
utils/pub/path.dart:320: parsed.parts[parsed.parts.length - 1] = split[0];
On 2012/12/11 02:09:42, nweiz wrote:
> Calling a private method here seems like a code smell. Why can't you do
> `parsed.parts[parsed.parts.length - 1] = parsed.filenameWithoutExtension`
> instead?

Done, though I don't think there's anything wrong with using a private here.
Privacy is library-level and it's all in the same library. I like library (as
opposed to class privacy). I also like "friend" in C++. :)

https://codereview.chromium.org/11512011/diff/1/utils/pub/path.dart#newcode506
utils/pub/path.dart:506: // If there is a dot (and it's not the first character,
like '.bashrc').
On 2012/12/11 02:09:42, nweiz wrote:
> This looks like it's supposed to go a couple lines further down.

Reworded the comment.

https://codereview.chromium.org/11512011/diff/1/utils/pub/path.dart#newcode509
utils/pub/path.dart:509: parts[parts.length - 1] = file.substring(0, lastDot);
On 2012/12/11 02:09:42, nweiz wrote:
> Why is this useful? Doesn't this mean that calling `parsed.extension` or
> `parsed.filenameWithoutExtension` will destructively modify `parsed`?

Eek! Copy/paste bug. Removed.

[nweiz, 2012-12-11 02:34:46]

lgtm

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

https://codereview.chromium.org/11512011/diff/1/utils/pub/path.dart#newcode316
utils/pub/path.dart:316: if (parsed.hasTrailingSeparator) return
parsed.toString();
On 2012/12/11 02:24:59, Bob Nystrom wrote:
> This is how Python works.

But not Ruby :).

> I think it would be confusing if
> withoutExtension("foo.bar/") returned "foo". I believe the expectation is that
> it will strip the extension from the file name, if there is a file at the end
of
> the path. If you have a trailing separator, I don't think there's a file at
the
> end of the path.

I disagree. I believe the expectation is that it will strip the extension from
the entry pointed to be the path (in this case, "foo.bar").

I think it's easier to see for the dual, extension(). It's relatively common
that directories will have meaningful extensions that users will want to
extract, and it's also relatively common that paths come directly from user
input. We could be introducing subtle bugs where "some_command foo.ext" on the
command line works, but "some_command foo.ext/" does not.

Here's a very real-world example: some Git servers use the ".git" suffix on
directories to determine if a directory is supposed to be Git. It would be
potentially confusing if such a server were sensitive to the presence of a slash
when finding directories.

It also forces users (and you, the API designer) to consider every API's
behavior with respect to trailing slashes. For example, does dirname("foo/bar")
return "foo" or "foo/"? Why?

[Bob Nystrom, 2012-12-11 02:47:01]

Thanks!

When we get some more time, I'm up for a discussion on the overall
strategy/model for trailing separators. I'd like to have a good answer there.

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

https://codereview.chromium.org/11512011/diff/1/utils/pub/path.dart#newcode316
utils/pub/path.dart:316: if (parsed.hasTrailingSeparator) return
parsed.toString();
On 2012/12/11 02:34:46, nweiz wrote:
> On 2012/12/11 02:24:59, Bob Nystrom wrote:
> > This is how Python works.
> 
> But not Ruby :).
> 
> > I think it would be confusing if
> > withoutExtension("foo.bar/") returned "foo". I believe the expectation is
that
> > it will strip the extension from the file name, if there is a file at the
end
> of
> > the path. If you have a trailing separator, I don't think there's a file at
> the
> > end of the path.
> 
> I disagree. I believe the expectation is that it will strip the extension from
> the entry pointed to be the path (in this case, "foo.bar").
> 
> I think it's easier to see for the dual, extension(). It's relatively common
> that directories will have meaningful extensions that users will want to
> extract, and it's also relatively common that paths come directly from user
> input. We could be introducing subtle bugs where "some_command foo.ext" on the
> command line works, but "some_command foo.ext/" does not.
> 
> Here's a very real-world example: some Git servers use the ".git" suffix on
> directories to determine if a directory is supposed to be Git. It would be
> potentially confusing if such a server were sensitive to the presence of a
slash
> when finding directories.
> 
> It also forces users (and you, the API designer) to consider every API's
> behavior with respect to trailing slashes. For example, does
dirname("foo/bar")
> return "foo" or "foo/"? Why?

I'm more interested here in what dirname("foo/bar/") returns. Python and Ruby
diverge there too: Python returns "foo/bar" and Ruby "foo".

I'm OK with changing this, but we should change it consistently across all
methods. They should all ignore trailing separators or treat them as meaningful
(essentially as denoting a path with a trailing unnamed file).