Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Future proofing 5.0.0 EventKind with #[non_exhaustive] #311

Open
erickt opened this issue May 11, 2021 · 3 comments
Open

Future proofing 5.0.0 EventKind with #[non_exhaustive] #311

erickt opened this issue May 11, 2021 · 3 comments
Labels
A-discussion A-enhancement Z-needs implementation Needs an implementation, will accept PRs
Milestone

Comments

@erickt
Copy link
Contributor

erickt commented May 11, 2021

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.

@0xpr03
Copy link
Member

0xpr03 commented May 15, 2021

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
Copy link
Member

0xpr03 commented May 15, 2021

(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..)

@erickt
Copy link
Contributor Author

erickt commented May 17, 2021

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.

@0xpr03 0xpr03 added this to the 5.0.0 milestone Aug 10, 2022
@0xpr03 0xpr03 added the Z-needs implementation Needs an implementation, will accept PRs label Aug 10, 2022
@0xpr03 0xpr03 modified the milestones: 5.0.0, 6.0.0 Aug 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-discussion A-enhancement Z-needs implementation Needs an implementation, will accept PRs
Projects
None yet
Development

No branches or pull requests

2 participants