Add isWithin to pkg/path.

Add isWithin to pkg/path.

R=rnystrom@google.com
BUG=14980
Committed: https://code.google.com/p/dart/source/detail?r=30317

[Bob Nystrom, 2013-11-14 22:58:58]

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

https://codereview.chromium.org/59483008/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:307: ///     path.isWithin('/root/path', '/root/other');
// -> false
Add an example for

path.isWith('/root/path', '/root/path');

https://codereview.chromium.org/59483008/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:757: } on ArgumentError catch (_) {
relative() can throw an ArgumentError?

We really shouldn't be catching that. It should either throw an exception, or
there should be a way to proactively detect if it would fail.

[nweiz, 2013-11-14 23:51:55]

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

https://codereview.chromium.org/59483008/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:307: ///     path.isWithin('/root/path', '/root/other');
// -> false
On 2013/11/14 22:58:59, Bob Nystrom wrote:
> Add an example for
> 
> path.isWith('/root/path', '/root/path');

Done.

https://codereview.chromium.org/59483008/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:757: } on ArgumentError catch (_) {
On 2013/11/14 22:58:59, Bob Nystrom wrote:
> relative() can throw an ArgumentError?

Yes. If [root] is relative, you can get unresolvable cases. For example, if
[root] and [path] are both "." and [from] is "/" or "..", there's no correct
return value.

> We really shouldn't be catching that. It should either throw an exception, or
> there should be a way to proactively detect if it would fail.

I'm not sure I agree. I don't think there's a clean way to express "detect
whether relative will fail" other than [canFindRelativePath], and no clean way
to implement it other than calling [relative] and catching the error.

I'm pretty sure this shouldn't throw an exception. `isWithin(".", "/foo")`
should return false even if [root] is ".". A relative root indicates that there
is no meaningful absolute root, which means that relative paths aren't beneath
any absolute paths.

[Bob Nystrom, 2013-11-15 00:05:51]

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

https://codereview.chromium.org/59483008/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:757: } on ArgumentError catch (_) {
On 2013/11/14 23:51:55, nweiz wrote:
> On 2013/11/14 22:58:59, Bob Nystrom wrote:
> > relative() can throw an ArgumentError?
> 
> Yes. If [root] is relative, you can get unresolvable cases. For example, if
> [root] and [path] are both "." and [from] is "/" or "..", there's no correct
> return value.
> 
> > We really shouldn't be catching that. It should either throw an exception,
or
> > there should be a way to proactively detect if it would fail.
> 
> I'm not sure I agree. I don't think there's a clean way to express "detect
> whether relative will fail" other than [canFindRelativePath], and no clean way
> to implement it other than calling [relative] and catching the error.

Right. In that case, relative() should throw a subclass of Exception, not
ArgumentError. The latter indicates a programmatic error, and should never be
caught (outside of logging, etc.). Maybe FormatException?

> 
> I'm pretty sure this shouldn't throw an exception. `isWithin(".", "/foo")`
> should return false even if [root] is ".". A relative root indicates that
there
> is no meaningful absolute root, which means that relative paths aren't beneath
> any absolute paths.

SGTM. I definitely prefer stuff here to not throw exceptions, so I'm fine with
this catching and eating it. I just don't think it should catch something of
type ArgumentError.

[nweiz, 2013-11-15 01:17:50]

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

https://codereview.chromium.org/59483008/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:757: } on ArgumentError catch (_) {
On 2013/11/15 00:05:51, Bob Nystrom wrote:
> On 2013/11/14 23:51:55, nweiz wrote:
> > On 2013/11/14 22:58:59, Bob Nystrom wrote:
> > > relative() can throw an ArgumentError?
> > 
> > Yes. If [root] is relative, you can get unresolvable cases. For example, if
> > [root] and [path] are both "." and [from] is "/" or "..", there's no correct
> > return value.
> > 
> > > We really shouldn't be catching that. It should either throw an exception,
> or
> > > there should be a way to proactively detect if it would fail.
> > 
> > I'm not sure I agree. I don't think there's a clean way to express "detect
> > whether relative will fail" other than [canFindRelativePath], and no clean
way
> > to implement it other than calling [relative] and catching the error.
> 
> Right. In that case, relative() should throw a subclass of Exception, not
> ArgumentError. The latter indicates a programmatic error, and should never be
> caught (outside of logging, etc.). Maybe FormatException?

I don't like FormatException, because nothing's being parsed or interpreted. I'm
going to add a PathException class for this.

> > 
> > I'm pretty sure this shouldn't throw an exception. `isWithin(".", "/foo")`
> > should return false even if [root] is ".". A relative root indicates that
> there
> > is no meaningful absolute root, which means that relative paths aren't
beneath
> > any absolute paths.
> 
> SGTM. I definitely prefer stuff here to not throw exceptions, so I'm fine with
> this catching and eating it. I just don't think it should catch something of
> type ArgumentError.

[Bob Nystrom, 2013-11-15 01:44:51]

SGTM. LGTM!

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

https://codereview.chromium.org/59483008/diff/1/pkg/path/lib/path.dart#newcod...
pkg/path/lib/path.dart:757: } on ArgumentError catch (_) {
On 2013/11/15 01:17:50, nweiz wrote:
> On 2013/11/15 00:05:51, Bob Nystrom wrote:
> > On 2013/11/14 23:51:55, nweiz wrote:
> > > On 2013/11/14 22:58:59, Bob Nystrom wrote:
> > > > relative() can throw an ArgumentError?
> > > 
> > > Yes. If [root] is relative, you can get unresolvable cases. For example,
if
> > > [root] and [path] are both "." and [from] is "/" or "..", there's no
correct
> > > return value.
> > > 
> > > > We really shouldn't be catching that. It should either throw an
exception,
> > or
> > > > there should be a way to proactively detect if it would fail.
> > > 
> > > I'm not sure I agree. I don't think there's a clean way to express "detect
> > > whether relative will fail" other than [canFindRelativePath], and no clean
> way
> > > to implement it other than calling [relative] and catching the error.
> > 
> > Right. In that case, relative() should throw a subclass of Exception, not
> > ArgumentError. The latter indicates a programmatic error, and should never
be
> > caught (outside of logging, etc.). Maybe FormatException?
> 
> I don't like FormatException, because nothing's being parsed or interpreted.
I'm
> going to add a PathException class for this.
> 
> > > 
> > > I'm pretty sure this shouldn't throw an exception. `isWithin(".", "/foo")`
> > > should return false even if [root] is ".". A relative root indicates that
> > there
> > > is no meaningful absolute root, which means that relative paths aren't
> beneath
> > > any absolute paths.
> > 
> > SGTM. I definitely prefer stuff here to not throw exceptions, so I'm fine
with
> > this catching and eating it. I just don't think it should catch something of
> > type ArgumentError.
> 

I was thinking the same thing. We should document that relative() can throw that
too.

[nweiz, 2013-11-15 22:05:22]

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