RFC: File watch events refactor

[blakejohnson]

This refactors out the UV_READABLE and UV_WRITABLE constants so that they no longer need to be imported. Instead, there are two new types FileEvent and FDEvent. The callback parameter order has also been shuffled so that status is always the last argument.

Note, the first few commits are the same as #4210.

[StefanKarpinski]

I think I'd prefer if these used a single integer flags field to store the flag state and provide accessor functions. This is a place where having bit fields like C would be really nice since we could get both at the same time.

[blakejohnson]

so, define functions like readable(f::FDEvent) = f.flags[1], etc.?

[StefanKarpinski]

That's kind of what I was thinking, but input from @JeffBezanson, @loladiro, @vtjnash & co. would be helpful. This is kind of a taste thing – for APIs like this I tend to favor a more C-like style than is traditional in other high-level languages.

[vtjnash]

seems like we need @enum_mask #2988 (with a better names) :)

[StefanKarpinski]

@flags?

[blakejohnson]

Ok, here it is with integer flag fields and accessors. One thing I couldn't figure out: I wanted to wrap the const definitions and accessors (UV_CHANGE, UV_RENAME, etc.) in a let block so that these definitions wouldn't clobber later definitions of the same name, but then the accessor definitions are not available in the outer scope. For instance, both event types have a TIMEDOUT property that could have a different bitmask. I chose unique names to avoid this trap, but it seems like there should be a way to make the const definitions internal to the type methods.

[StefanKarpinski]

I definitely think I prefer that. Would be good to get some feedback from @loladiro, @vtjnash & @JeffBezanson.

[JeffBezanson]

I disagree. There's no need to manipulate integer flags so much when they are purely a detail of how the underlying C function needs to be called. I'd rather not export 3 new function names just to access specific bit fields that the user doesn't need to know about. I wrote a version of it here: 146d391, on top of Blake's changes. It requires fewer definitions and fewer exports.

[blakejohnson]

The user shouldn't have to know the specifics of how the bit fields are implemented, but we do want users to be able to write callback functions that accept these event types as inputs, and to use the results of wait. So, it really comes down to whether user code should look like:

fm = watch_file("afile.txt")
_, e = wait(fm)
if e.changed
    ....

or whether that last line should look like

if changed(e)

Both look fine to me. In either case, we probably need to export the new event types.

[JeffBezanson]

I would err on the side of minimizing the number of exported names. Exporting the event types might also be optional, since you can check e.changed without knowing the type of e.

(FWIW I have a similar objection to the methods of ispath, isfile, uperm, gperm, etc. that take integers, since it doesn't really make sense to ask whether an integer is a file. But at least the stat struct is somewhat traditional.)

[blakejohnson]

@JeffBezanson Sure, but it seems odd to expect users to write a callback accepting e, but if they want to type-annotate the input, they have to use e::Base.FileEvent. That's fine, but we definitely have to document it.

I don't have a strong preference on fieldnames vs. accessor methods, so whatever you guys decide is fine with me.

[StefanKarpinski]

This whole API strikes me as a bit clunky, but maybe that's just the nature of the beast :-\