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

Make the output channel generic #249

Open
1 task
vincent-herlemont opened this issue Oct 6, 2024 · 11 comments
Open
1 task

Make the output channel generic #249

vincent-herlemont opened this issue Oct 6, 2024 · 11 comments
Labels
help wanted Open issue for anyone who wants to work on it. R&D Research and development
Milestone

Comments

@vincent-herlemont
Copy link
Owner

Currently, event listening via the watch command does not support generic channel management. The user is forced to choose between the tokio feature or the standard Rust channel (see: src/watch/mod.rs#L38-L46). The purpose of this issue is to investigate whether it is possible to elegantly support any type of channel.

TODO

  • Investigate
@vincent-herlemont vincent-herlemont added the help wanted Open issue for anyone who wants to work on it. label Oct 6, 2024
@vincent-herlemont vincent-herlemont added this to the 0.9.0 milestone Oct 6, 2024
@vincent-herlemont vincent-herlemont added the R&D Research and development label Oct 11, 2024
@lu-zero
Copy link

lu-zero commented Nov 12, 2024

The less than elegant solution is to rely on flume or another crate supporting both sync and async at the same time. If you think it is acceptable I can provide a PR

@vincent-herlemont
Copy link
Owner Author

@lu-zero this could save a feature. However, the goal here is to find a way to be compatible with all types of channels to avoid using multiple types of channels in a crate.

@lu-zero
Copy link

lu-zero commented Nov 13, 2024

There isn't a common trait across all the channel implementation so there isn't a way even to support alternative implementations of sync channels like flume vs crossbeam vs std or async channels such as flume vs tokio vs async-channel short of providing the trait, assume that there is a sender and take a generic over it (and still have to differentiate between sync and async).

@vincent-herlemont
Copy link
Owner Author

vincent-herlemont commented Nov 13, 2024

Yes, you are right, there won't be any generics in terms of Rust declarations. However, we could instead find a way to make it "generic" by using a Fn (that takes the event as an argument), which will be defined by the users, as a parameter and is called every time an event is sent. In this case, users will be able to use these events as they wish, with channels or something else if they want.

@lu-zero
Copy link

lu-zero commented Nov 14, 2024

Storing Fn requires a generic and tends to be annoying in my experience though...

@vincent-herlemont
Copy link
Owner Author

vincent-herlemont commented Nov 14, 2024

Storing Fn requires a generic

@lu-zero How does this imply a generic declaration? Here's what I imagined, what's your point of view?

// /!\ Pseudo code
let (sender, receiver) = mpsc::channel();
db.watch(move |event| {
    // Send the event message into the channel
   sender.send(event);
});

As you can see, you can put whatever you want in the closure. This does not imply a generic.

@lu-zero
Copy link

lu-zero commented Nov 14, 2024

Fn/FnMut are traits and closures are always unique.

@vincent-herlemont
Copy link
Owner Author

@lu-zero Can you provide a detailed example of a situation where this would pose a problem?

@lu-zero
Copy link

lu-zero commented Nov 14, 2024

It is easier to write down the signature of your pseudocode I guess :)

// `clo` is generic over anything implementing `FnMut`
fn watch(&mut self, clo: impl FnMut(EventTy)) {

// and if we want to save it someplace we have to box it
self.closure = Box::new(clo);

...
}  

This should be a way to make it work for sync, for async it gets more annoying since we do not have yet async closures.

@vincent-herlemont
Copy link
Owner Author

vincent-herlemont commented Nov 14, 2024

@lu-zero Yes, that's exactly the idea! And indeed, for async, we'll have to wait a bit ;)

BTW If you need support for flume or something else in the meantime, you can add it to the two existing channel supports, and I will merge it.

@lu-zero
Copy link

lu-zero commented Nov 14, 2024

I do not need it myself, but since it is just few minutes to implement it using flume, it could solve the use-case of keeping the same API across features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Open issue for anyone who wants to work on it. R&D Research and development
Projects
None yet
Development

No branches or pull requests

2 participants