Fix Path.relativeTo and add support for relative paths as input.

Fix Path.relativeTo and add support for relative paths as input.


BUG=https://code.google.com/p/dart/issues/detail?id=7870
BUG=https://code.google.com/p/dart/issues/detail?id=7873
TEST=standalone/io/path_test

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

[Bill Hesse, 2013-01-16 12:54:03]

There is a one-line change in test_suite.dart needed, when the Dart executable
in tools/testing/bin is updated.  Where should we keep track of that?

[Mads Ager (google), 2013-01-16 13:20:10]

The code looks good. I have one higher-level question. Going forward, maybe we
need to just have path manipulation at two different levels. One for URIs in the
dart:uri library and one for file paths (potentially just a set of top-level
functions on strings).

https://codereview.chromium.org/11968012/diff/1/tests/standalone/io/path_test...
File tests/standalone/io/path_test.dart (right):

https://codereview.chromium.org/11968012/diff/1/tests/standalone/io/path_test...
tests/standalone/io/path_test.dart:194: // Trailing slash in the base path has
no effect.  This matches Path.join
I'm wondering if this is actually the right thing to do. We are currently
internally operating on URI paths, so maybe we should stay with the URI
semantics here and just be better about documenting it (and have an easy way to
add the trailing slash if we know we are dealing with a directory).

https://codereview.chromium.org/11968012/diff/1/tests/standalone/io/path_test...
tests/standalone/io/path_test.dart:245: new Path('/a/b').relativeTo(new
Path('c/d')));
I think this will fit on one line?

https://codereview.chromium.org/11968012/diff/1/tests/standalone/io/path_test...
tests/standalone/io/path_test.dart:248: new Path('a/b').relativeTo(new
Path('/a/b')));
Ditto.

[Bill Hesse, 2013-01-16 13:32:55]

https://codereview.chromium.org/11968012/diff/1/tests/standalone/io/path_test...
File tests/standalone/io/path_test.dart (right):

https://codereview.chromium.org/11968012/diff/1/tests/standalone/io/path_test...
tests/standalone/io/path_test.dart:194: // Trailing slash in the base path has
no effect.  This matches Path.join
On 2013/01/16 13:20:10, Mads Ager wrote:
> I'm wondering if this is actually the right thing to do. We are currently
> internally operating on URI paths, so maybe we should stay with the URI
> semantics here and just be better about documenting it (and have an easy way
to
> add the trailing slash if we know we are dealing with a directory).

I see your point.  But I would not like to leave it as-is without changing .join
to have URI semantics as well.  We should keep considering this.

With the new semantics, it turns out that the URI semantics can be easily
restored by calling
a.relativeTo(b.directoryPath)
instead of
a.relativeTo(b).

This is so straightforward that I will put it in the documentation of relativeTo
in the Path interface.

[Mads Ager (google), 2013-01-16 13:47:28]

LGTM

I see you point about join as well. Thanks for adding the extra documentation.
Have you gone through all uses of relativeTo in the repo to check that they do
not expect the URI semantics of relativeTo?

[Bob Nystrom, 2013-01-16 18:42:38]

I'm glad to see this operation getting more robust but it makes me sad to see
this much wheel re-invention going on. If you really want to flesh out the Path
class, why not copy code from pkg/path? It implements this fully with lots of
tests, docs, corner cases, etc.

Have you considered at least using it as a reference?

https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart
File sdk/lib/io/path_impl.dart (right):

https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart#...
sdk/lib/io/path_impl.dart:75: while (common < length && pathSegments[common] ==
baseSegments[common]) {
I believe this will do the wrong thing if you have drive letters that differ by
case or slash style.

https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart#...
sdk/lib/io/path_impl.dart:84: "  Arguments: $_path.relativeTo($base)");
Throwing an ArgumentError here is a trip-wire for the user. They can't
programmatically prevent this unless they reimplement the same logic that this
method contains, and they shouldn't catch it because it's an Error.

https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart#...
sdk/lib/io/path_impl.dart:103: "  Arguments: $_path.relativeTo($base)");
Ditto here. This will give users a bad day.

pkg/path (like Python and Ruby) handle this by assuming a relative path is
relative to the cwd if that larger context is needed.

[Bill Hesse, 2013-01-18 14:17:13]

On 2013/01/16 18:42:38, Bob Nystrom wrote:
> I'm glad to see this operation getting more robust but it makes me sad to see
> this much wheel re-invention going on. If you really want to flesh out the
Path
> class, why not copy code from pkg/path? It implements this fully with lots of
> tests, docs, corner cases, etc.
> 
> Have you considered at least using it as a reference?
> 
> https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart
> File sdk/lib/io/path_impl.dart (right):
> 
>
https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart#...
> sdk/lib/io/path_impl.dart:75: while (common < length && pathSegments[common]
==
> baseSegments[common]) {
> I believe this will do the wrong thing if you have drive letters that differ
by
> case or slash style.
> 
>
https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart#...
> sdk/lib/io/path_impl.dart:84: "  Arguments: $_path.relativeTo($base)");
> Throwing an ArgumentError here is a trip-wire for the user. They can't
> programmatically prevent this unless they reimplement the same logic that this
> method contains, and they shouldn't catch it because it's an Error.
> 
>
https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart#...
> sdk/lib/io/path_impl.dart:103: "  Arguments: $_path.relativeTo($base)");
> Ditto here. This will give users a bad day.
> 
> pkg/path (like Python and Ruby) handle this by assuming a relative path is
> relative to the cwd if that larger context is needed.


I will fix the issues reported here, and a few formatting comments from Mads
that I missed, and we have a plan for working together now in the future.  I
prefer to make the impossible relative path an exception that users should
catch, and fix the case on drive letters (they can never differ by slash
direction - there is only one slash direction).

[Bob Nystrom, 2013-01-18 16:07:28]

On 2013/01/18 14:17:13, Bill Hesse wrote:
> On 2013/01/16 18:42:38, Bob Nystrom wrote:
> > I'm glad to see this operation getting more robust but it makes me sad to
see
> > this much wheel re-invention going on. If you really want to flesh out the
> Path
> > class, why not copy code from pkg/path? It implements this fully with lots
of
> > tests, docs, corner cases, etc.
> > 
> > Have you considered at least using it as a reference?
> > 
> > https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart
> > File sdk/lib/io/path_impl.dart (right):
> > 
> >
>
https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart#...
> > sdk/lib/io/path_impl.dart:75: while (common < length && pathSegments[common]
> ==
> > baseSegments[common]) {
> > I believe this will do the wrong thing if you have drive letters that differ
> by
> > case or slash style.
> > 
> >
>
https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart#...
> > sdk/lib/io/path_impl.dart:84: "  Arguments: $_path.relativeTo($base)");
> > Throwing an ArgumentError here is a trip-wire for the user. They can't
> > programmatically prevent this unless they reimplement the same logic that
this
> > method contains, and they shouldn't catch it because it's an Error.
> > 
> >
>
https://codereview.chromium.org/11968012/diff/1006/sdk/lib/io/path_impl.dart#...
> > sdk/lib/io/path_impl.dart:103: "  Arguments: $_path.relativeTo($base)");
> > Ditto here. This will give users a bad day.
> > 
> > pkg/path (like Python and Ruby) handle this by assuming a relative path is
> > relative to the cwd if that larger context is needed.
> 
> 
> I will fix the issues reported here, and a few formatting comments from Mads
> that I missed, and we have a plan for working together now in the future. 

Sounds great! Let me know if there's anything I can do to help.

- bob