notify icon indicating copy to clipboard operation
notify copied to clipboard

Future proofing 5.0.0 EventKind with #[non_exhaustive]

Open erickt opened this issue 4 years ago • 3 comments

More follow on to #306, what do you think about marking all the EventKind enum types, and child enums as #[non_exhaustive]? Looking at both the fsevents flags and inotify manpage, it looks like the event type expands over time, so I think it'd be worth allowing new event types to be added over time.

I think this would be preferable over using Event::set_info, at least in most cases, since it can be difficult for downstream users to understand the stability guarantee of the info strings, especially since they are not documented.

erickt avatar May 11 '21 03:05 erickt

Apparently I didn't send my thoughts on this..

I think it's ok to mark it as non_exuastive, though I'm not sure what the exact consequences for users is. I'm not sure if you could call it counter position, but in other discussions people mentioned that we would probably steer towards anonymous events. Meaning that there are users that don't care about the exact type and just need the knowledge that anything happened.

0xpr03 avatar May 15 '21 16:05 0xpr03

(The current API in general is, depending on which issue you read, completely wrong or completely what the people want, see also my paused attempt at re-introducing the debouncer..)

0xpr03 avatar May 15 '21 16:05 0xpr03

I don't have firm opinions on this either (especially since I haven't actually had to use this library for real yet :) ). The main consequence would be that users would have to write their event type handlers as:

match event.kind {
    EventKind::Any => { ... },
    EventKind::Access(_) => { ... },
    EventKind::Create(_) => { ... },
    EventKind::Modify(_) => { ... },
    EventKind::Remove(_) => { ... },
    EventKind::Other => { ... }
    // always need to handle the case that we got some event that we don't know about.
    _ => { ... }
}

And likewise for the other sub-kinds.

The other strategy would be to declare constants for the fsevents info() (and other future users of info()). It's not perfect, but it helps to ameliorate the risk that these info strings change out from under a user. Or going even further, we could change the Other field to be parameterized by a Box<dyn Any>, which would be some watcher-specific field, but then we need to pay for the box for non-standard things.

Like I said though, I don't have enough experience with notify, and all the underlying OS implementations, to have a great recommendation, so I'd be fine letting this percolate over time.

erickt avatar May 17 '21 17:05 erickt