Wrap Directory.watch on Mac OS for the watcher package.

Wrap Directory.watch on Mac OS for the watcher package.

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

[Bob Nystrom, 2013-11-09 00:42:57]

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
File pkg/watcher/lib/src/directory_watcher/mac_os.dart (right):

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:20: /// FSEvents has two main
idiosynchrasies that this class works around. First,
"idiosyncrasies"

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:104: if (events.isEmpty)
events = _eventsBasedOnFileSystem(path);
Why set to an empty list above just to change it something else here?

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:139: /// contain any events
relating to [directory].
"[directory]" is not a parameter here.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:146: batch =
batch.where((event) => event.path != directory).toList();
Is the .toList() needed here?

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:156:
isInModifiedDirectory(path) =>
Nit: blank line above this please.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:201: /// single event that
describes that happened to the path in question.
"that happened" -> "what happened".

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:216: // doesn't, that's a
contradiction so we should return null.
Remove "so we should return null".

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/path_set....
File pkg/watcher/lib/src/path_set.dart (right):

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/path_set....
pkg/watcher/lib/src/path_set.dart:47: for (var i = 0; i < parts.length; i++) {
Use a for-in loop here?

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/path_set....
pkg/watcher/lib/src/path_set.dart:63: 
This could definitely use some more docs.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/path_set....
pkg/watcher/lib/src/path_set.dart:72: if (entry.isEmpty &&
!_paths.contains(partialPath)) dir.remove(part);
// If after removing the children, there is nothing left, remove this too.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/path_set....
pkg/watcher/lib/src/path_set.dart:149: if (!p.isRelative(relative) ||
parts.first == '..' || parts.first == '.') {
We should really add isWithin() to pkg/path.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/test/path_set_tes...
File pkg/watcher/test/path_set_test.dart (right):

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/test/path_set_tes...
pkg/watcher/test/path_set_test.dart:13: 'set contains "$path"');
I think this shadows an imported contains() function in unittest. How about
containsPath?

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/test/path_set_tes...
pkg/watcher/test/path_set_test.dart:40: expect(set,
contains("root/path/to/file"));
Is the argument to contains() normalized too? Should it be?

[nweiz, 2013-11-09 02:19:25]

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
File pkg/watcher/lib/src/directory_watcher/mac_os.dart (right):

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:20: /// FSEvents has two main
idiosynchrasies that this class works around. First,
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> "idiosyncrasies"

Ten Signs You've Been Writing The Word "Asynchronous" Too Often

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:104: if (events.isEmpty)
events = _eventsBasedOnFileSystem(path);
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> Why set to an empty list above just to change it something else here?

Good point. Changed.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:139: /// contain any events
relating to [directory].
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> "[directory]" is not a parameter here.

It is a field on the class, though.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:146: batch =
batch.where((event) => event.path != directory).toList();
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> Is the .toList() needed here?

Since Iterables are lazy, the [where] will be executed again and again whenever
we call any method on [batch]. Using [toList] means it's only executed once.

Also I'd have to rename the variable if I didn't make it a list, since [batch]
is typed as a List.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:156:
isInModifiedDirectory(path) =>
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> Nit: blank line above this please.

Done.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:201: /// single event that
describes that happened to the path in question.
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> "that happened" -> "what happened".

Done.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:216: // doesn't, that's a
contradiction so we should return null.
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> Remove "so we should return null".

Done.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/directory...
pkg/watcher/lib/src/directory_watcher/mac_os.dart:216: // doesn't, that's a
contradiction so we should return null.
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> Remove "so we should return null".

Done.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/path_set....
File pkg/watcher/lib/src/path_set.dart (right):

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/path_set....
pkg/watcher/lib/src/path_set.dart:47: for (var i = 0; i < parts.length; i++) {
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> Use a for-in loop here?

Done. This used to be structured differently.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/path_set....
pkg/watcher/lib/src/path_set.dart:63: 
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> This could definitely use some more docs.

Done.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/path_set....
pkg/watcher/lib/src/path_set.dart:72: if (entry.isEmpty &&
!_paths.contains(partialPath)) dir.remove(part);
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> // If after removing the children, there is nothing left, remove this too.

Done.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/lib/src/path_set....
pkg/watcher/lib/src/path_set.dart:149: if (!p.isRelative(relative) ||
parts.first == '..' || parts.first == '.') {
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> We should really add isWithin() to pkg/path.

Filed a bug and referenced it here.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/test/path_set_tes...
File pkg/watcher/test/path_set_test.dart (right):

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/test/path_set_tes...
pkg/watcher/test/path_set_test.dart:13: 'set contains "$path"');
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> I think this shadows an imported contains() function in unittest. How about
> containsPath?

Done.

https://codereview.chromium.org/66163002/diff/1/pkg/watcher/test/path_set_tes...
pkg/watcher/test/path_set_test.dart:40: expect(set,
contains("root/path/to/file"));
On 2013/11/09 00:42:57, Bob Nystrom wrote:
> Is the argument to contains() normalized too? Should it be?

Yes; that's tested in the "contains()" group below.

[Bob Nystrom, 2013-11-11 18:44:08]

LGTM!

[nweiz, 2013-11-11 19:47:31]

Committed patchset #2 manually as r30168 (presubmit successful).