Change the way Patterns work in scheduled_test/descriptor.

Change the way Patterns work in scheduled_test/descriptor.

Instead of every Entry being able to have a pattern as a name, now there's a
separate Pattern entry that encapsulates the logic of finding entries that match
a pattern.

The Pattern class also has different semantics than the old pattern names. The
old semantics required only one filesystem entity with a name matching the
pattern; now any number are allowed, as long as only one of them matches the
Entry (that is, has the expected contents).

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

[Bob Nystrom, 2013-03-15 21:58:46]

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/descri...
File pkg/scheduled_test/lib/descriptor.dart (right):

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/descri...
pkg/scheduled_test/lib/descriptor.dart:118: Pattern pattern(core.Pattern name,
EntryCreator fn) => new Pattern(name, fn);
Eek, reusing a name from dart:core seems like a really bad idea. I know this
library will usually be used with a prefix, but that doesn't help callstacks and
other places where this name may appear.

Given that you rarely use the type name directly anyway, how about
PatternDescriptor (and probably ditto for the other Descriptor types)?

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/src/de...
File pkg/scheduled_test/lib/src/descriptor/file.dart (right):

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/src/de...
pkg/scheduled_test/lib/src/descriptor/file.dart:100:
"${prefixLines(expected)}\n"
Indent +2.

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/src/de...
File pkg/scheduled_test/lib/src/descriptor/pattern.dart (right):

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/src/de...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:114: throw "Pattern
descriptors don't support create().";
UnsupportedError.

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/src/de...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:118: "support load().");
Is there a reason this propagates the error through the future where create()
throws synchronously?

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/des...
File pkg/scheduled_test/lib/descriptor.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/des...
pkg/scheduled_test/lib/descriptor.dart:118: Pattern pattern(core.Pattern name,
EntryCreator fn) => new Pattern(name, fn);
It's pretty verbose at callsites to always have to do new RegExp(). How about
making the first parameter a string and then automatically wrap it in a RegExp?

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/descriptor/async.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/async.dart:31: Future validate([String
parent]) => schedule(() => validateNow(parent));
Can this just be the default implementation of validate() in Entry?

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/descriptor/file.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/file.dart:100:
"${prefixLines(expected)}\n"
Indent +2.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/descriptor/pattern.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:53: .map((entry) => entry is
File ? entry.fullPathSync() : entry.path)
Does this work with the new Link stuff too?

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:58: if
(matchingEntries.length == 0) {
.isEmpty

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:72: if (matches.length == 1)
return;
This works, but you may want to add a .toList() after results.where() so you
don't keep traversing the iterable multiple times.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:114: throw "Pattern
descriptors don't support create().";
Throw UnsupportedError.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:118: "support load().");
Is there a reason you're returning the error asynchronously here but
synchronously above?

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/utils.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/utils.dart:35: String prefixLines(String text,
{String prefix: '| ', String firstPrefix}) {
This feels like ab arbitrary default prefix. At the callsites I couldn't intuit
what the prefix would be when this was omitted. Maybe not make it optional?

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/utils.dart:43: lines.insertRange(0, 1, firstLine);
insert(0, firstLine)

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/test/de...
File pkg/scheduled_test/test/descriptor_test.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/test/de...
pkg/scheduled_test/test/descriptor_test.dart:757: d.pattern(new RegExp(r'f..'),
(name) => d.file(name, 'blap')).validate();
I like pulling the pattern stuff out of File and Directory, but the use sites
now are pretty verbose and confusing. How about making convenience methods:

filePattern(String pattern, String contents) => pattern(new RegExp(pattern),
(name) => file(name, contents);

and ditto for directory?

[nweiz, 2013-03-15 23:17:21]

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/descri...
File pkg/scheduled_test/lib/descriptor.dart (right):

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/descri...
pkg/scheduled_test/lib/descriptor.dart:118: Pattern pattern(core.Pattern name,
EntryCreator fn) => new Pattern(name, fn);
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> Eek, reusing a name from dart:core seems like a really bad idea. I know this
> library will usually be used with a prefix, but that doesn't help callstacks
and
> other places where this name may appear.
> 
> Given that you rarely use the type name directly anyway, how about
> PatternDescriptor (and probably ditto for the other Descriptor types)?

We already talked about this for the other descriptor types. It's really awkward
to refer to descriptor.FileDescriptor.

It's a little more of an edge case with a name defined in dart:core, but I still
think this name is fine. We're using a language with built-in namespacing
capabilities; I think we should take advantage of them to choose the best named
in the context of the library, rather than adding manual namespacing on top.

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/src/de...
File pkg/scheduled_test/lib/src/descriptor/file.dart (right):

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/src/de...
pkg/scheduled_test/lib/src/descriptor/file.dart:100:
"${prefixLines(expected)}\n"
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> Indent +2.

Done.

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/src/de...
File pkg/scheduled_test/lib/src/descriptor/pattern.dart (right):

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/src/de...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:114: throw "Pattern
descriptors don't support create().";
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> UnsupportedError.

Done.

https://codereview.chromium.org/12853005/diff/1/pkg/scheduled_test/lib/src/de...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:118: "support load().");
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> Is there a reason this propagates the error through the future where create()
> throws synchronously?

Good point, create() should be in a Future.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/des...
File pkg/scheduled_test/lib/descriptor.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/des...
pkg/scheduled_test/lib/descriptor.dart:118: Pattern pattern(core.Pattern name,
EntryCreator fn) => new Pattern(name, fn);
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> It's pretty verbose at callsites to always have to do new RegExp(). How about
> making the first parameter a string and then automatically wrap it in a
RegExp?

I don't like disallowing other Patterns. I think the correct solution here would
be less-verbose RegExps, but given that that's not the Dart Way I don't want to
sacrifice flexibility here.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/descriptor/async.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/async.dart:31: Future validate([String
parent]) => schedule(() => validateNow(parent));
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> Can this just be the default implementation of validate() in Entry?

I want to encourage subclasses to add descriptions to their schedule() calls.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/descriptor/file.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/file.dart:100:
"${prefixLines(expected)}\n"
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> Indent +2.

Done.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/descriptor/pattern.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:53: .map((entry) => entry is
File ? entry.fullPathSync() : entry.path)
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> Does this work with the new Link stuff too?

Probably not. I'll add a TODO.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:58: if
(matchingEntries.length == 0) {
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> .isEmpty

Done.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:72: if (matches.length == 1)
return;
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> This works, but you may want to add a .toList() after results.where() so you
> don't keep traversing the iterable multiple times.

Done.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:114: throw "Pattern
descriptors don't support create().";
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> Throw UnsupportedError.

Done.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/pattern.dart:118: "support load().");
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> Is there a reason you're returning the error asynchronously here but
> synchronously above?

Good point, create() should be in a Future.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/utils.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/utils.dart:35: String prefixLines(String text,
{String prefix: '| ', String firstPrefix}) {
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> This feels like ab arbitrary default prefix. At the callsites I couldn't
intuit
> what the prefix would be when this was omitted. Maybe not make it optional?

I think of this as a utility method for visually setting apart a block of text,
rather than one for performing a specific string operation. "|" is a good
default from this perspective. I'd rather change the name to make the intent
more obvious than make the parameter required, but I can't think of a good name
to change it to.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/utils.dart:43: lines.insertRange(0, 1, firstLine);
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> insert(0, firstLine)

Woah, we have insert() now?

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/test/de...
File pkg/scheduled_test/test/descriptor_test.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/test/de...
pkg/scheduled_test/test/descriptor_test.dart:757: d.pattern(new RegExp(r'f..'),
(name) => d.file(name, 'blap')).validate();
On 2013/03/15 21:58:46, Bob Nystrom wrote:
> I like pulling the pattern stuff out of File and Directory, but the use sites
> now are pretty verbose and confusing. How about making convenience methods:
> 
> filePattern(String pattern, String contents) => pattern(new RegExp(pattern),
> (name) => file(name, contents);
> 
> and ditto for directory?

Done.

[Bob Nystrom, 2013-03-18 21:06:34]

LGTM with the longer __Descriptor type names.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/des...
File pkg/scheduled_test/lib/descriptor.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/des...
pkg/scheduled_test/lib/descriptor.dart:118: Pattern pattern(core.Pattern name,
EntryCreator fn) => new Pattern(name, fn);
On 2013/03/15 23:17:21, nweiz wrote:
> On 2013/03/15 21:58:46, Bob Nystrom wrote:
> > It's pretty verbose at callsites to always have to do new RegExp(). How
about
> > making the first parameter a string and then automatically wrap it in a
> RegExp?
> 
> I don't like disallowing other Patterns. I think the correct solution here
would
> be less-verbose RegExps, but given that that's not the Dart Way I don't want
to
> sacrifice flexibility here.

I like that Pattern (er, I mean descriptor.Pattern) is flexible and takes a
(dart:core) Pattern. But for the top-level DSL function, I think terseness is
its main job. I think given that, it might make sense for it to implicitly mean
"RegExp".

Another thing I worry about is that currently, it *will* accept a string
(because it implements Pattern), but then it won't do what you expect.

Maybe another option is to have a different top-level method for "regexp pattern
descriptor"?

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/descriptor/async.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/descriptor/async.dart:31: Future validate([String
parent]) => schedule(() => validateNow(parent));
On 2013/03/15 23:17:21, nweiz wrote:
> On 2013/03/15 21:58:46, Bob Nystrom wrote:
> > Can this just be the default implementation of validate() in Entry?
> 
> I want to encourage subclasses to add descriptions to their schedule() calls.

Good point.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
File pkg/scheduled_test/lib/src/utils.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/utils.dart:35: String prefixLines(String text,
{String prefix: '| ', String firstPrefix}) {
On 2013/03/15 23:17:21, nweiz wrote:
> On 2013/03/15 21:58:46, Bob Nystrom wrote:
> > This feels like ab arbitrary default prefix. At the callsites I couldn't
> intuit
> > what the prefix would be when this was omitted. Maybe not make it optional?
> 
> I think of this as a utility method for visually setting apart a block of
text,
> rather than one for performing a specific string operation. "|" is a good
> default from this perspective. I'd rather change the name to make the intent
> more obvious than make the parameter required, but I can't think of a good
name
> to change it to.

SGTM. I don't have a better name either.

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/src...
pkg/scheduled_test/lib/src/utils.dart:43: lines.insertRange(0, 1, firstLine);
On 2013/03/15 23:17:21, nweiz wrote:
> On 2013/03/15 21:58:46, Bob Nystrom wrote:
> > insert(0, firstLine)
> 
> Woah, we have insert() now?

http://api.dartlang.org/docs/bleeding_edge/dart_core/List.html#insert

Like a real programming language!

[nweiz, 2013-03-18 21:41:05]

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/des...
File pkg/scheduled_test/lib/descriptor.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/des...
pkg/scheduled_test/lib/descriptor.dart:118: Pattern pattern(core.Pattern name,
EntryCreator fn) => new Pattern(name, fn);
On 2013/03/18 21:06:34, Bob Nystrom wrote:
> I like that Pattern (er, I mean descriptor.Pattern) is flexible and takes a
> (dart:core) Pattern. But for the top-level DSL function, I think terseness is
> its main job. I think given that, it might make sense for it to implicitly
mean
> "RegExp".

"new d.PatternDescriptor(..., (name) => d.dir(name, ...))" is exceptionally
verbose compared to "d.dirPattern(..., ...)". Especially given that patterns are
relatively rarely-used anyway, I'd like to make all the uses mostly terse rather
than splitting them into very terse and very verbose.

> Another thing I worry about is that currently, it *will* accept a string
> (because it implements Pattern), but then it won't do what you expect.

I'm pretty sure if you pass a String to a method named "pattern", you'll expect
it to behave the same way it would behave when you pass it to any other method
that takes a Pattern. I think this is a feature.

> Maybe another option is to have a different top-level method for "regexp
pattern
> descriptor"?

I'd rather not make a combinatorial explosion of method names, and I don't think
"regExpDirPattern(..., ...)" is much better than "dirPattern(new RegExp(...),
...)".

[Bob Nystrom, 2013-03-18 22:05:34]

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/des...
File pkg/scheduled_test/lib/descriptor.dart (right):

https://codereview.chromium.org/12853005/diff/3001/pkg/scheduled_test/lib/des...
pkg/scheduled_test/lib/descriptor.dart:118: Pattern pattern(core.Pattern name,
EntryCreator fn) => new Pattern(name, fn);
On 2013/03/18 21:41:05, nweiz wrote:
> On 2013/03/18 21:06:34, Bob Nystrom wrote:
> > I like that Pattern (er, I mean descriptor.Pattern) is flexible and takes a
> > (dart:core) Pattern. But for the top-level DSL function, I think terseness
is
> > its main job. I think given that, it might make sense for it to implicitly
> mean
> > "RegExp".
> 
> "new d.PatternDescriptor(..., (name) => d.dir(name, ...))" is exceptionally
> verbose compared to "d.dirPattern(..., ...)". Especially given that patterns
are
> relatively rarely-used anyway, I'd like to make all the uses mostly terse
rather
> than splitting them into very terse and very verbose.
> 
> > Another thing I worry about is that currently, it *will* accept a string
> > (because it implements Pattern), but then it won't do what you expect.
> 
> I'm pretty sure if you pass a String to a method named "pattern", you'll
expect
> it to behave the same way it would behave when you pass it to any other method
> that takes a Pattern. I think this is a feature.
> 
> > Maybe another option is to have a different top-level method for "regexp
> pattern
> > descriptor"?
> 
> I'd rather not make a combinatorial explosion of method names, and I don't
think
> "regExpDirPattern(..., ...)" is much better than "dirPattern(new RegExp(...),
> ...)".

Ah, forgot about filePattern() and dirPattern(). SGTM!

[nweiz, 2013-03-18 23:24:21]

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