Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18612013:
File watching package. (Closed)
|
Created:
7 years, 5 months ago by Bob Nystrom Modified:
7 years, 5 months ago CC:
reviews_dartlang.org, Siggi Cherem (dart-lang) Visibility:
Public. |
DescriptionFile watching package.
BUG=
R=nweiz@google.com
Committed: https://code.google.com/p/dart/source/detail?r=24971
Patch Set 1 #
Total comments: 84
Patch Set 2 : Clean up import. #Patch Set 3 : Revised and added example script. #
Total comments: 4
Patch Set 4 : Revise. #Patch Set 5 : Remove reference to old unittest config. #
Total comments: 2
Patch Set 6 : Revise. #Patch Set 7 : "pathos" -> "path" #
Messages
Total messages: 12 (0 generated)
CC Siggy in case he has any feedback since he's implemented something similar.
just a couple drive-by-comments https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:30: StreamController<WatchEvent> _events; change to final? https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:116: if (lastStatus == null || !_sameHash(lastStatus.hash, hash)) { consider making this a configuration option - some applications might want to be notified on every change, even if the contents are identical. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:135: bool _sameHash(List<int> a, List<int> b) { alternatively call CrytoUtils.bytesToHex and compare them as strings? https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart File pkg/watcher/test/utils.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:15: // TODO(rnystrom): Find a better way to use this. move the config to unittest? For now, make sure this package is excluded from the script that publish packages from trunk to pub.
https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:30: StreamController<WatchEvent> _events; On 2013/07/10 22:53:53, Siggi Cherem (dart-lang) wrote: > change to final? Wish I could. :( The only way to set onListen and onCancel on a StreamController is in its constructor, and the functions I'm providing there need access to the DirectoryWatcher. This means it has to be called in the constructor *body*, which is too late for final. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:116: if (lastStatus == null || !_sameHash(lastStatus.hash, hash)) { On 2013/07/10 22:53:53, Siggi Cherem (dart-lang) wrote: > consider making this a configuration option - some applications might want to be > notified on every change, even if the contents are identical. I expect this to get more configurable over time, but I figured for the first patch I'd start simple. I can always add options, it's hard to take them away. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:135: bool _sameHash(List<int> a, List<int> b) { On 2013/07/10 22:53:53, Siggi Cherem (dart-lang) wrote: > alternatively call CrytoUtils.bytesToHex and compare them as strings? Hmm, is that faster? Seems like a roundabout way to do it. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart File pkg/watcher/test/utils.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:15: // TODO(rnystrom): Find a better way to use this. On 2013/07/10 22:53:53, Siggi Cherem (dart-lang) wrote: > move the config to unittest? It depends on stack_trace and, I think, some other stuff. Is it OK for unittest to depend on those? > For now, make sure this package is excluded from the script that publish > packages from trunk to pub. I know it's nasty, but I'm personally OK with letting this be published. You can still use the package, you just can't run its tests. I can remove this line if you'd prefer.
https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart File pkg/watcher/test/utils.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:15: // TODO(rnystrom): Find a better way to use this. On 2013/07/10 23:04:39, Bob Nystrom wrote: > On 2013/07/10 22:53:53, Siggi Cherem (dart-lang) wrote: > > move the config to unittest? > > It depends on stack_trace and, I think, some other stuff. Is it OK for unittest > to depend on those? Eventually yes, I believe Nathan was already working on using stack_strace in unittest, but he had to roll it back temporarily. I can't recall why. > > > For now, make sure this package is excluded from the script that publish > > packages from trunk to pub. > > I know it's nasty, but I'm personally OK with letting this be published. You can > still use the package, you just can't run its tests. I can remove this line if > you'd prefer. I'm ok either way. I slightly prefer removing it to make it self contained.
https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/change_ty... File pkg/watcher/lib/src/change_type.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/change_ty... pkg/watcher/lib/src/change_type.dart:10: static const ADD = const ChangeType(0); I think it's a little cleaner to use the name of the enum value rather than an opaque integer (as we do in e.g. log.dart in pub). https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:16: /// Watches the contents of a directory and emits [WatchEvents] when something "[WatchEvent]s" https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:22: /// The [Stream] of file modification events that have occurred to files in "file modification events" -> "events" Also, mention that this is a broadcast stream, since the type doesn't communicate that. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:29: Style nit: I'd get rid of this newline to indicate that [events] and its controller are paired. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:30: StreamController<WatchEvent> _events; I'd call this _eventsController. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:35: /// files have been modified. Style nit: first sentence should be its own paragraph. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:56: recursive: true, followLinks: true); I'd much rather we use pub's listDirectory here, either adapted to be asynchronous or just used synchronously. [Directory.list] still screws up edge cases when symlinks are involved. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:68: }).then((_) { I think the following would be a little cleaner here (gets rid of the weirdly-scoped [futures] variable): return stream.map((entity) { if (entity is! File) return new Future.value(); files.add(entity.path); return _refreshFile(entity.path); }).then((futures) { return Future.wait(futures); }) https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:71: if (_state.shouldNotify) { It's a little weird that this is a flag, but you just explicitly check that _state is watching below. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:90: return _watch(); Shouldn't this just be "return;"? Otherwise, wouldn't [watch] get called twice? If you change this to "return", you can get rid of the [shouldWatch] check above. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:106: return false; Is this return value used anywhere? https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:113: if (!_state.shouldNotify) return; Fold this into the following "if", or vice versa. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:162: watcher._watch(); If you move this into DirectoryWatcher, then _WatchState won't need to access any private members of DirectoryWatcher, so you can put it into its own library. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:179: /// now we're waiting for changes to come in. "waiting for changes to come in" -> "polling for changes" https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:188: assert(false); If it's possible, it would be nice to provide some metadata here about what event was received and which state received it. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/stat.dart File pkg/watcher/lib/src/stat.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/stat.dart... pkg/watcher/lib/src/stat.dart:10: typedef DateTime MockTimeCallback(String path); Document this. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/stat.dart... pkg/watcher/lib/src/stat.dart:14: /// Overrided the default behavior for accessing a file's modification time with "Overrided" -> "Override" https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/watch_eve... File pkg/watcher/lib/src/watch_event.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/watch_eve... pkg/watcher/lib/src/watch_event.dart:12: final ChangeType type; I'm usually a big advocate for splitting out classes into separate libraries, but for enums I actually think it's a little cleaner to define them in the same file they're used. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/directory_wa... File pkg/watcher/test/directory_watcher_test.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/directory_wa... pkg/watcher/test/directory_watcher_test.dart:7: import 'dart:async'; // temp Remove temp import https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart File pkg/watcher/test/utils.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:15: // TODO(rnystrom): Find a better way to use this. On 2013/07/10 23:25:31, Siggi Cherem (dart-lang) wrote: > Eventually yes, I believe Nathan was already working on using stack_strace in > unittest, but he had to roll it back temporarily. I can't recall why. It caused a VM crash. asiva@ is working on it. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:22: var _nextEvent = 0; It would be nice to document all of these variables, but [_nextEvent] in particular is unclear without further context. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:26: /// The actual file system has pretty high granularity for file modification Do you mean "low granularity"? https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:45: _ensureSandbox(); Is calling this everywhere really cleaner than creating a wrapper for [test] like we do in pub, or using [setUp]? https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:58: schedule(() => new Future.delayed(new Duration(milliseconds: 50))); Explicit timeouts are very dangerous when dealing with the bots, since they're so much slower than our computers. I'm confident 50ms won't be enough here. Another possibility here is to use a sentinel file. Keep modifying a single file in the sandbox directory until that modification is reported; once it is, we know the watcher is initialized. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:70: // the event gets dropped before we receive it. "the event gets dropped" -> "drop the event" https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:73: }), completes); expect() doesn't return a Future. I think you want to use currentSchedule.wrapFuture here instead. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:81: _ensureSandbox(); This seems redundant, since [createWatcher] must be called before this anyway. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:99: void writeFile(String path, {String contents, bool updateModified}) { This should be scheduled, as should removeFile and renameFile. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:153: } This function isn't called anywhere. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:157: if (_sandboxDir == null) { Feels like this should short-circuit. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:163: path = pathos.relative(path, from: _sandboxDir); Add an assert that this works.
https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/change_ty... File pkg/watcher/lib/src/change_type.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/change_ty... pkg/watcher/lib/src/change_type.dart:10: static const ADD = const ChangeType(0); On 2013/07/11 00:25:01, nweiz wrote: > I think it's a little cleaner to use the name of the enum value rather than an > opaque integer (as we do in e.g. log.dart in pub). I think I heard one of the VM guys say numbers are faster or something. <shrug> Changed to strings. I like that too. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:16: /// Watches the contents of a directory and emits [WatchEvents] when something On 2013/07/11 00:25:01, nweiz wrote: > "[WatchEvent]s" Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:22: /// The [Stream] of file modification events that have occurred to files in On 2013/07/11 00:25:01, nweiz wrote: > "file modification events" -> "events" > > Also, mention that this is a broadcast stream, since the type doesn't > communicate that. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:29: On 2013/07/11 00:25:01, nweiz wrote: > Style nit: I'd get rid of this newline to indicate that [events] and its > controller are paired. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:30: StreamController<WatchEvent> _events; On 2013/07/11 00:25:01, nweiz wrote: > I'd call this _eventsController. Since the stream itself isn't stored, I think that's a little painfully verbose. I'll leave it as-is if that's OK. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:35: /// files have been modified. On 2013/07/11 00:25:01, nweiz wrote: > Style nit: first sentence should be its own paragraph. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:56: recursive: true, followLinks: true); On 2013/07/11 00:25:01, nweiz wrote: > I'd much rather we use pub's listDirectory here, either adapted to be > asynchronous or just used synchronously. [Directory.list] still screws up edge > cases when symlinks are involved. I think it's important for watcher to use a stream API for this, and modifying pub's listDirectory() to be stream-based would be a big chunk of work. dart:io's handling of symlinks is much better than it used to be, and they're responding to bugs much faster. I'd like to try just using the vanilla API and push on them when/if we run into problems. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:68: }).then((_) { On 2013/07/11 00:25:01, nweiz wrote: > I think the following would be a little cleaner here (gets rid of the > weirdly-scoped [futures] variable): > > return stream.map((entity) { > if (entity is! File) return new Future.value(); > files.add(entity.path); > return _refreshFile(entity.path); > }).then((futures) { > return Future.wait(futures); > }) Had to stick a toList() in there, but this is a big improvement. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:71: if (_state.shouldNotify) { On 2013/07/11 00:25:01, nweiz wrote: > It's a little weird that this is a flag, but you just explicitly check that > _state is watching below. Changed to check shouldNotify below. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:90: return _watch(); On 2013/07/11 00:25:01, nweiz wrote: > Shouldn't this just be "return;"? Otherwise, wouldn't [watch] get called twice? > > If you change this to "return", you can get rid of the [shouldWatch] check > above. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:106: return false; On 2013/07/11 00:25:01, nweiz wrote: > Is this return value used anywhere? Oops. Not anymore. Used to have the later future stuff popped out a level. Removed. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:113: if (!_state.shouldNotify) return; On 2013/07/11 00:25:01, nweiz wrote: > Fold this into the following "if", or vice versa. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:162: watcher._watch(); On 2013/07/11 00:25:01, nweiz wrote: > If you move this into DirectoryWatcher, You mean move the state instances? > then _WatchState won't need to access > any private members of DirectoryWatcher, so you can put it into its own library. _WatchState is deeply tied to DirectoryWatcher, so I actually like that it's in the same library. I don't think we need strict one-class-per-library style. If this was C++, they'd be friend classes. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:179: /// now we're waiting for changes to come in. On 2013/07/11 00:25:01, nweiz wrote: > "waiting for changes to come in" -> "polling for changes" Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:188: assert(false); On 2013/07/11 00:25:01, nweiz wrote: > If it's possible, it would be nice to provide some metadata here about what > event was received and which state received it. I just went ahead and deleted this. It only exist(s|ed) to trap programmatic errors in the state machine, but it's so simple that I don't think it's worth it. Now, you'll get a NPE if you try to send an event (listen, cancel, finish) on a state that doesn't expect that. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/stat.dart File pkg/watcher/lib/src/stat.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/stat.dart... pkg/watcher/lib/src/stat.dart:10: typedef DateTime MockTimeCallback(String path); On 2013/07/11 00:25:01, nweiz wrote: > Document this. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/stat.dart... pkg/watcher/lib/src/stat.dart:14: /// Overrided the default behavior for accessing a file's modification time with On 2013/07/11 00:25:01, nweiz wrote: > "Overrided" -> "Override" "Overrides". Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/watch_eve... File pkg/watcher/lib/src/watch_event.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/watch_eve... pkg/watcher/lib/src/watch_event.dart:12: final ChangeType type; On 2013/07/11 00:25:01, nweiz wrote: > I'm usually a big advocate for splitting out classes into separate libraries, > but for enums I actually think it's a little cleaner to define them in the same > file they're used. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/directory_wa... File pkg/watcher/test/directory_watcher_test.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/directory_wa... pkg/watcher/test/directory_watcher_test.dart:7: import 'dart:async'; // temp On 2013/07/11 00:25:01, nweiz wrote: > Remove temp import Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart File pkg/watcher/test/utils.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:15: // TODO(rnystrom): Find a better way to use this. On 2013/07/10 23:25:31, Siggi Cherem (dart-lang) wrote: > On 2013/07/10 23:04:39, Bob Nystrom wrote: > > On 2013/07/10 22:53:53, Siggi Cherem (dart-lang) wrote: > > > move the config to unittest? > > > > It depends on stack_trace and, I think, some other stuff. Is it OK for > unittest > > to depend on those? > > Eventually yes, I believe Nathan was already working on using stack_strace in > unittest, but he had to roll it back temporarily. I can't recall why. > > > > > > > For now, make sure this package is excluded from the script that publish > > > packages from trunk to pub. > > > > I know it's nasty, but I'm personally OK with letting this be published. You > can > > still use the package, you just can't run its tests. I can remove this line if > > you'd prefer. > > I'm ok either way. I slightly prefer removing it to make it self contained. Yup, I'll remove this and switch it out to use compact config once that lands. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:22: var _nextEvent = 0; On 2013/07/11 00:25:01, nweiz wrote: > It would be nice to document all of these variables, but [_nextEvent] in > particular is unclear without further context. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:26: /// The actual file system has pretty high granularity for file modification On 2013/07/11 00:25:01, nweiz wrote: > Do you mean "low granularity"? Damn ambiguity of directional metaphors! Changed to "coarse". https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:45: _ensureSandbox(); On 2013/07/11 00:25:01, nweiz wrote: > Is calling this everywhere really cleaner than creating a wrapper for [test] > like we do in pub, or using [setUp]? I kind of like it. Wrapping test() is a bit tedious because you have to hide the other import and then handle both test() and solo_test(). I could make initConfig() call setUp(), but that feels a bit magical, and does unnecessary work if there are later unit tests that don't need the sandbox. I like that this only creates a sandbox for tests that use it. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:58: schedule(() => new Future.delayed(new Duration(milliseconds: 50))); On 2013/07/11 00:25:01, nweiz wrote: > Explicit timeouts are very dangerous when dealing with the bots, since they're > so much slower than our computers. I'm confident 50ms won't be enough here. I know. My plan was to tune it upwards until the bots were reliably stable. Yes, it's gross. :( > Another possibility here is to use a sentinel file. Keep modifying a single file > in the sandbox directory until that modification is reported; once it is, we > know the watcher is initialized. I started hacking on that, but the code is pretty long and nasty. It also interferes with the behavior under test: if it's waiting for events then you can't write tests for things like the first event, or what happens before events are sent. Another possible option would be have a private API (like src/stat.dart) that exposes a stream that emits an object when the initial scan is done. This could then wait for that. That would be reliable and simple, but I'm not crazy about adding code to the library that exists just for testing. Thoughts? https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:70: // the event gets dropped before we receive it. On 2013/07/11 00:25:01, nweiz wrote: > "the event gets dropped" -> "drop the event" Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:73: }), completes); On 2013/07/11 00:25:01, nweiz wrote: > expect() doesn't return a Future. I think you want to use > currentSchedule.wrapFuture here instead. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:81: _ensureSandbox(); On 2013/07/11 00:25:01, nweiz wrote: > This seems redundant, since [createWatcher] must be called before this anyway. Removed. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:99: void writeFile(String path, {String contents, bool updateModified}) { On 2013/07/11 00:25:01, nweiz wrote: > This should be scheduled, as should removeFile and renameFile. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:153: } On 2013/07/11 00:25:01, nweiz wrote: > This function isn't called anywhere. Oops. Old code. Removed. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:157: if (_sandboxDir == null) { On 2013/07/11 00:25:01, nweiz wrote: > Feels like this should short-circuit. Done. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:163: path = pathos.relative(path, from: _sandboxDir); On 2013/07/11 00:25:01, nweiz wrote: > Add an assert that this works. Done.
https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:56: recursive: true, followLinks: true); On 2013/07/11 18:33:20, Bob Nystrom wrote: > On 2013/07/11 00:25:01, nweiz wrote: > > I'd much rather we use pub's listDirectory here, either adapted to be > > asynchronous or just used synchronously. [Directory.list] still screws up edge > > cases when symlinks are involved. > > I think it's important for watcher to use a stream API for this, and modifying > pub's listDirectory() to be stream-based would be a big chunk of work. dart:io's > handling of symlinks is much better than it used to be, and they're responding > to bugs much faster. I'd like to try just using the vanilla API and push on them > when/if we run into problems. They've marked an important bug as wontfix: https://code.google.com/p/dart/issues/detail?id=4794. Recursive symlinks are an edge case, sure, but the [Directory.list] behavior in that edge case is bad enough that it's likely to cause serious and subtle bugs for anyone who runs into it. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:162: watcher._watch(); On 2013/07/11 18:33:20, Bob Nystrom wrote: > On 2013/07/11 00:25:01, nweiz wrote: > > If you move this into DirectoryWatcher, > > You mean move the state instances? No, I mean the call to "watcher._watch" in particular. > _WatchState is deeply tied to DirectoryWatcher, so I actually like that it's in > the same library. I don't think we need strict one-class-per-library style. If > this was C++, they'd be friend classes. I don't think the one-class-per-library style should be strict, but I do think it generally makes code easier to read, both by limiting the size of files and encouraging a use of private names that communicates more information. As I mentioned above, this line is the only one in the class where WatchState refers to a private member of DirectoryWatcher. In fact, it's the only one where WatchState refers to DirectoryWatcher at all. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart File pkg/watcher/test/utils.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:45: _ensureSandbox(); On 2013/07/11 18:33:20, Bob Nystrom wrote: > On 2013/07/11 00:25:01, nweiz wrote: > > Is calling this everywhere really cleaner than creating a wrapper for [test] > > like we do in pub, or using [setUp]? > > I kind of like it. Wrapping test() is a bit tedious because you have to hide the > other import and then handle both test() and solo_test(). > > I could make initConfig() call setUp(), but that feels a bit magical, and does > unnecessary work if there are later unit tests that don't need the sandbox. You should just call [setUp] explicitly. You only have one test file; there's no need for unnecessary abstraction to support multiple files that don't exist yet. > I like that this only creates a sandbox for tests that use it. Every test uses it. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:58: schedule(() => new Future.delayed(new Duration(milliseconds: 50))); On 2013/07/11 18:33:20, Bob Nystrom wrote: > On 2013/07/11 00:25:01, nweiz wrote: > > Explicit timeouts are very dangerous when dealing with the bots, since they're > > so much slower than our computers. I'm confident 50ms won't be enough here. > > I know. My plan was to tune it upwards until the bots were reliably stable. Yes, > it's gross. :( > > > Another possibility here is to use a sentinel file. Keep modifying a single > file > > in the sandbox directory until that modification is reported; once it is, we > > know the watcher is initialized. > > I started hacking on that, but the code is pretty long and nasty. It also > interferes with the behavior under test: if it's waiting for events then you > can't write tests for things like the first event, or what happens before events > are sent. > > Another possible option would be have a private API (like src/stat.dart) that > exposes a stream that emits an object when the initial scan is done. This could > then wait for that. > > That would be reliable and simple, but I'm not crazy about adding code to the > library that exists just for testing. Thoughts? Given the state of the bots, I think pretty much anything is better than using a timeout to wait for processing to be done. It won't just involve tweaking this CL multiple times on submit; it'll involve re-tweaking it in the future each time the bots get slower. It actually may be a good idea to expose an onReady event anyway. For example, in barback, we probably only want to run the initial compile once the first scan has been completed. If we start compiling before that, we run the risk of a file changing after we read it but before the initial scan reaches it. https://codereview.chromium.org/18612013/diff/13001/pkg/watcher/lib/src/direc... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/13001/pkg/watcher/lib/src/direc... pkg/watcher/lib/src/directory_watcher.dart:83: if (_state.shouldNotify) { Should this be "previousState.shouldNotify"? If not, "previousState" is an unused variable. https://codereview.chromium.org/18612013/diff/13001/pkg/watcher/lib/src/direc... pkg/watcher/lib/src/directory_watcher.dart:190: /// If a change event should be sent for a file modification while in this "If" -> "Whether" https://codereview.chromium.org/18612013/diff/13001/pkg/watcher/test/utils.dart File pkg/watcher/test/utils.dart (right): https://codereview.chromium.org/18612013/diff/13001/pkg/watcher/test/utils.da... pkg/watcher/test/utils.dart:27: /// The index in [_watcher]'s event stream for the next event. When event Paragraph break. https://codereview.chromium.org/18612013/diff/13001/pkg/watcher/test/utils.da... pkg/watcher/test/utils.dart:84: currentSchedule.wrapFuture(future); This is meant to wrap a Future; `future` should be assigned to its output.
https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:56: recursive: true, followLinks: true); On 2013/07/11 22:29:00, nweiz wrote: > On 2013/07/11 18:33:20, Bob Nystrom wrote: > > On 2013/07/11 00:25:01, nweiz wrote: > > > I'd much rather we use pub's listDirectory here, either adapted to be > > > asynchronous or just used synchronously. [Directory.list] still screws up > edge > > > cases when symlinks are involved. > > > > I think it's important for watcher to use a stream API for this, and modifying > > pub's listDirectory() to be stream-based would be a big chunk of work. > dart:io's > > handling of symlinks is much better than it used to be, and they're responding > > to bugs much faster. I'd like to try just using the vanilla API and push on > them > > when/if we run into problems. > > They've marked an important bug as wontfix: > https://code.google.com/p/dart/issues/detail?id=4794. Recursive symlinks are an > edge case, sure, but the [Directory.list] behavior in that edge case is bad > enough that it's likely to cause serious and subtle bugs for anyone who runs > into it. I think you marked that bug WontFix. I changed this to not follow symlinks which should avoid that issue. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:162: watcher._watch(); On 2013/07/11 22:29:00, nweiz wrote: > On 2013/07/11 18:33:20, Bob Nystrom wrote: > > On 2013/07/11 00:25:01, nweiz wrote: > > > If you move this into DirectoryWatcher, > > > > You mean move the state instances? > > No, I mean the call to "watcher._watch" in particular. > > > _WatchState is deeply tied to DirectoryWatcher, so I actually like that it's > in > > the same library. I don't think we need strict one-class-per-library style. If > > this was C++, they'd be friend classes. > > I don't think the one-class-per-library style should be strict, but I do think > it generally makes code easier to read, both by limiting the size of files and > encouraging a use of private names that communicates more information. Agreed on both accounts. In this case, I think having both classes in the library makes sense: they are intimately tied. _WatchState is completely an implementation detail of DirectoryWatcher's current state. If Dart had nested classes, I would probably use one for this. > > As I mentioned above, this line is the only one in the class where WatchState > refers to a private member of DirectoryWatcher. In fact, it's the only one where > WatchState refers to DirectoryWatcher at all. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart File pkg/watcher/test/utils.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:45: _ensureSandbox(); On 2013/07/11 22:29:00, nweiz wrote: > On 2013/07/11 18:33:20, Bob Nystrom wrote: > > On 2013/07/11 00:25:01, nweiz wrote: > > > Is calling this everywhere really cleaner than creating a wrapper for [test] > > > like we do in pub, or using [setUp]? > > > > I kind of like it. Wrapping test() is a bit tedious because you have to hide > the > > other import and then handle both test() and solo_test(). > > > > I could make initConfig() call setUp(), but that feels a bit magical, and does > > unnecessary work if there are later unit tests that don't need the sandbox. > > You should just call [setUp] explicitly. You only have one test file; there's no > need for unnecessary abstraction to support multiple files that don't exist yet. > > > I like that this only creates a sandbox for tests that use it. > > Every test uses it. I'm certain more tests will exist and the current code works fine and isn't bad. Why is it worth changing it? https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:58: schedule(() => new Future.delayed(new Duration(milliseconds: 50))); On 2013/07/11 22:29:00, nweiz wrote: > On 2013/07/11 18:33:20, Bob Nystrom wrote: > > On 2013/07/11 00:25:01, nweiz wrote: > > > Explicit timeouts are very dangerous when dealing with the bots, since > they're > > > so much slower than our computers. I'm confident 50ms won't be enough here. > > > > I know. My plan was to tune it upwards until the bots were reliably stable. > Yes, > > it's gross. :( > > > > > Another possibility here is to use a sentinel file. Keep modifying a single > > file > > > in the sandbox directory until that modification is reported; once it is, we > > > know the watcher is initialized. > > > > I started hacking on that, but the code is pretty long and nasty. It also > > interferes with the behavior under test: if it's waiting for events then you > > can't write tests for things like the first event, or what happens before > events > > are sent. > > > > Another possible option would be have a private API (like src/stat.dart) that > > exposes a stream that emits an object when the initial scan is done. This > could > > then wait for that. > > > > That would be reliable and simple, but I'm not crazy about adding code to the > > library that exists just for testing. Thoughts? > > Given the state of the bots, I think pretty much anything is better than using a > timeout to wait for processing to be done. It won't just involve tweaking this > CL multiple times on submit; it'll involve re-tweaking it in the future each > time the bots get slower. > > It actually may be a good idea to expose an onReady event anyway. For example, > in barback, we probably only want to run the initial compile once the first scan > has been completed. If we start compiling before that, we run the risk of a file > changing after we read it but before the initial scan reaches it. Done. Calling it "ready" is nice and ambiguous and doesn't couple it to polling.
https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:56: recursive: true, followLinks: true); On 2013/07/12 00:31:09, Bob Nystrom wrote: > On 2013/07/11 22:29:00, nweiz wrote: > > On 2013/07/11 18:33:20, Bob Nystrom wrote: > > > On 2013/07/11 00:25:01, nweiz wrote: > > > > I'd much rather we use pub's listDirectory here, either adapted to be > > > > asynchronous or just used synchronously. [Directory.list] still screws up > > edge > > > > cases when symlinks are involved. > > > > > > I think it's important for watcher to use a stream API for this, and > modifying > > > pub's listDirectory() to be stream-based would be a big chunk of work. > > dart:io's > > > handling of symlinks is much better than it used to be, and they're > responding > > > to bugs much faster. I'd like to try just using the vanilla API and push on > > them > > > when/if we run into problems. > > > > They've marked an important bug as wontfix: > > https://code.google.com/p/dart/issues/detail?id=4794. Recursive symlinks are > an > > edge case, sure, but the [Directory.list] behavior in that edge case is bad > > enough that it's likely to cause serious and subtle bugs for anyone who runs > > into it. > > I think you marked that bug WontFix. Soren incorrectly marked it as fixed. I changed the status to accurately reflect the stated resolution. > I changed this to not follow symlinks which should avoid that issue. Are we okay with barback (and other potential users of this library) not supporting symlinked directories? https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:162: watcher._watch(); On 2013/07/12 00:31:09, Bob Nystrom wrote: > On 2013/07/11 22:29:00, nweiz wrote: > > On 2013/07/11 18:33:20, Bob Nystrom wrote: > > > On 2013/07/11 00:25:01, nweiz wrote: > > > > If you move this into DirectoryWatcher, > > > > > > You mean move the state instances? > > > > No, I mean the call to "watcher._watch" in particular. > > > > > _WatchState is deeply tied to DirectoryWatcher, so I actually like that it's > > in > > > the same library. I don't think we need strict one-class-per-library style. > If > > > this was C++, they'd be friend classes. > > > > I don't think the one-class-per-library style should be strict, but I do think > > it generally makes code easier to read, both by limiting the size of files and > > encouraging a use of private names that communicates more information. > > Agreed on both accounts. In this case, I think having both classes in the > library makes sense: they are intimately tied. _WatchState is completely an > implementation detail of DirectoryWatcher's current state. If Dart had nested > classes, I would probably use one for this. > > > > > As I mentioned above, this line is the only one in the class where WatchState > > refers to a private member of DirectoryWatcher. In fact, it's the only one > where > > WatchState refers to DirectoryWatcher at all. > Okay. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart File pkg/watcher/test/utils.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:45: _ensureSandbox(); On 2013/07/12 00:31:09, Bob Nystrom wrote: > On 2013/07/11 22:29:00, nweiz wrote: > > On 2013/07/11 18:33:20, Bob Nystrom wrote: > > > On 2013/07/11 00:25:01, nweiz wrote: > > > > Is calling this everywhere really cleaner than creating a wrapper for > [test] > > > > like we do in pub, or using [setUp]? > > > > > > I kind of like it. Wrapping test() is a bit tedious because you have to hide > > the > > > other import and then handle both test() and solo_test(). > > > > > > I could make initConfig() call setUp(), but that feels a bit magical, and > does > > > unnecessary work if there are later unit tests that don't need the sandbox. > > > > You should just call [setUp] explicitly. You only have one test file; there's > no > > need for unnecessary abstraction to support multiple files that don't exist > yet. > > > > > I like that this only creates a sandbox for tests that use it. > > > > Every test uses it. > > I'm certain more tests will exist and the current code works fine and isn't bad. Why will more tests exist? Are you planning on adding additional functionality? If those tests require the same setup, why can't they use the same setUp callback? If they don't, why not put them in a different file? > Why is it worth changing it? Because this way of writing it is worse than the alternative. It adds additional global state, it requires that you remember to put "_ensureSandbox" at the beginning of every utility function, and it's a custom-rolled way of doing something where the standard solution (setUp) would work just fine. https://codereview.chromium.org/18612013/diff/36001/pkg/watcher/lib/src/direc... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/36001/pkg/watcher/lib/src/direc... pkg/watcher/lib/src/directory_watcher.dart:187: }); Left-over formatting change?
https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/lib/src/directory... pkg/watcher/lib/src/directory_watcher.dart:56: recursive: true, followLinks: true); On 2013/07/12 01:04:28, nweiz wrote: > On 2013/07/12 00:31:09, Bob Nystrom wrote: > > On 2013/07/11 22:29:00, nweiz wrote: > > > On 2013/07/11 18:33:20, Bob Nystrom wrote: > > > > On 2013/07/11 00:25:01, nweiz wrote: > > > > > I'd much rather we use pub's listDirectory here, either adapted to be > > > > > asynchronous or just used synchronously. [Directory.list] still screws > up > > > edge > > > > > cases when symlinks are involved. > > > > > > > > I think it's important for watcher to use a stream API for this, and > > modifying > > > > pub's listDirectory() to be stream-based would be a big chunk of work. > > > dart:io's > > > > handling of symlinks is much better than it used to be, and they're > > responding > > > > to bugs much faster. I'd like to try just using the vanilla API and push > on > > > them > > > > when/if we run into problems. > > > > > > They've marked an important bug as wontfix: > > > https://code.google.com/p/dart/issues/detail?id=4794. Recursive symlinks are > > an > > > edge case, sure, but the [Directory.list] behavior in that edge case is bad > > > enough that it's likely to cause serious and subtle bugs for anyone who runs > > > into it. > > > > I think you marked that bug WontFix. > > Soren incorrectly marked it as fixed. I changed the status to accurately reflect > the stated resolution. > > > I changed this to not follow symlinks which should avoid that issue. > > Are we okay with barback (and other potential users of this library) not > supporting symlinked directories? Yes, for now I'm OK with not supported symlinked directories (or directories containing symlinks). We can always expand this functionality over time. https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart File pkg/watcher/test/utils.dart (right): https://codereview.chromium.org/18612013/diff/1/pkg/watcher/test/utils.dart#n... pkg/watcher/test/utils.dart:45: _ensureSandbox(); On 2013/07/12 01:04:28, nweiz wrote: > On 2013/07/12 00:31:09, Bob Nystrom wrote: > > On 2013/07/11 22:29:00, nweiz wrote: > > > On 2013/07/11 18:33:20, Bob Nystrom wrote: > > > > On 2013/07/11 00:25:01, nweiz wrote: > > > > > Is calling this everywhere really cleaner than creating a wrapper for > > [test] > > > > > like we do in pub, or using [setUp]? > > > > > > > > I kind of like it. Wrapping test() is a bit tedious because you have to > hide > > > the > > > > other import and then handle both test() and solo_test(). > > > > > > > > I could make initConfig() call setUp(), but that feels a bit magical, and > > does > > > > unnecessary work if there are later unit tests that don't need the > sandbox. > > > > > > You should just call [setUp] explicitly. You only have one test file; > there's > > no > > > need for unnecessary abstraction to support multiple files that don't exist > > yet. > > > > > > > I like that this only creates a sandbox for tests that use it. > > > > > > Every test uses it. > > > > I'm certain more tests will exist and the current code works fine and isn't > bad. > > Why will more tests exist? Are you planning on adding additional functionality? Probably. At the very least, I'm thinking of expanding this to have watchers for watching a collection of directories, or a set of fixed file paths. > If those tests require the same setup, why can't they use the same setUp > callback? If they don't, why not put them in a different file? They will be in a different file, but they'll still use utils.dart and initConfig(). > > > Why is it worth changing it? > > Because this way of writing it is worse than the alternative. It adds additional > global state, it requires that you remember to put "_ensureSandbox" at the > beginning of every utility function, and it's a custom-rolled way of doing > something where the standard solution (setUp) would work just fine. OK, done. My original issue was that initConfig() isn't the right place for this. So I added a separate createSandbox() function and made the test suite do setUp(createSandbox). That way the setUp() call is still in the test where it's clearly visible and other tests can call initConfig() without getting a sandbox they don't care about. https://codereview.chromium.org/18612013/diff/36001/pkg/watcher/lib/src/direc... File pkg/watcher/lib/src/directory_watcher.dart (right): https://codereview.chromium.org/18612013/diff/36001/pkg/watcher/lib/src/direc... pkg/watcher/lib/src/directory_watcher.dart:187: }); On 2013/07/12 01:04:29, nweiz wrote: > Left-over formatting change? Yup! Done.
lgtm
Message was sent while issue was closed.
Committed patchset #7 manually as r24971 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
