file system event watching fixes #4210
Conversation
| @@ -282,7 +286,7 @@ function poll_file(s, interval_seconds::Real, seconds::Real) | |||
| wait(wt) == :poll | |||
| end | |||
|
|
|||
| function watch_file(cb, s; poll=false) | |||
| function watch_file(cb=false, s; poll=false) | |||
There was a problem hiding this comment.
This doesn't work.
There was a problem hiding this comment.
Right, an accidental late addition. Fixing it now.
|
We need to come up with a better API than exposing uv internals. Other than that this is a good change. |
|
@loladiro I agree that the API needs more work. For starters, we could translate the libuv event types into some kind of |
|
Looks like the build is failing with my newly added test. I'll look into it (everything passes on my local machine). |
|
Ok, tests pass now. |
|
I'd prefer to tackle the API issues in a separate pull request. If you'd like me to rebase this before merging, let me know. |
Export watch_file, as this is the easiest API entry point between FileMonitor and PollingFileMonitor. Fix _uv_hook_fseventscb such that the condition is notified even if no callback is in the monitor. Also fix the value sent to notify() to match the expected return value in wait(). Fix watch_poll default callback.
|
I am uncomfortable adding API that we're gonna break anyway so close to a release. |
|
Yes the |
|
In this first pass, I was really just trying to fix what was already there. You'll notice that That said, I am happy to keep pushing forward with an API refactor in this pull request. Is that what you all would prefer? |
|
It seems to me that getting this merged and then fixing the API post haste is probably best. Maybe we should simply not export the new constants? |
|
@StefanKarpinski we could do that, and simply patch the test to reach into |
|
I think that's the best course of action – it doesn't break the API any more than it already is and fixes the problem. We can change the API separately. |
|
|
|
Updated to not export the new constants. |
|
Yes, they're certainly just as bad. The idea is that this change does no harm if it doesn't export anything new. We can unexport |
|
I made some progress tonight on a refactor that gets rid of (exported) |
|
I believe this is superseded by #4247. |
|
Sorry for making you do pointless work, @blakejohnson! |
Export
watch_file, as this is the easiest API entry point betweenFileMonitorand
PollingFileMonitor.Fix
_uv_hook_fseventscbsuch that theConditionis notified even if no callbackis in the
FileMonitor. Also fix the value sent tonotifyto match the expectedreturn value in
wait.