Request for feedback for my new crate

code review
#1

Hello fellow Rustaceans,
This week I have been working on my first crate, edisp, an enum dispatcher. This crate provides traits required to perform dispatching depending on enum variants yielded by an iterator. This allows, for instance, to get every errors and every successes from an iterator instead of stopping at the first error. Macros are also provided to automatically implement required traits to dispatch for user-defined enums.

The crate is available on github. I want to expand it a bit more before its first release and publishing it on crates.io. So far, I have the following questions:

  • are information given on the readme concise and clear enough?
  • are the docs and examples correctly written and easily understandable?
  • are there some alternative crates that may be cited?
  • are tests correctly written?

This crate uses a custom trait entitled Container, which allows to unify behavior of various collections, such as Vec, HashMap, and so on. I think other type of collections defined in other crates can implement this trait by adding features to edisp. For instance, Container may be implemented on SmallVec if the smallvec feature is enabled in the cargo file. Do you think that would be a nice improvement?

Do not hesitate to share your critics as replies of this topic, or feel free to open issues on github.

Have a nice day,
Léo

#2

Awesome crate!

I don't know of other existing crates which do similar things, though now I wonder why. Looks pretty useful!

Looking at the interface, though, I have a few suggestions:

  • As a user, I'd prefer a proc_macro derive for Dispatch, rather than a macro_rules! macro. Specifically, if I could just write #[derive(Dispatch)], then I wouldn't have to repeat my enum's name, generics, and variants twice, once in a separate call.

  • Have you considered replacing Container with a set of existing traits? It looks roughly equivalent to requiring Default + Extend, and if you used those traits then you'd get edisp automatically working with all std containers + most external custom containers, as most of them will implement both Default and Extend. There would be no need for smallvec, as you mention, to depend on edisp then.

    Having a single Container trait would be nice, but it would have the significant downside of requiring everything to implement it, whereas if you used existing traits, you could get those impls automatically.

Those were my two biggest notes. This looks super nice, though! Even if you didn't resolve those, I'd recommend posting a early version (maybe 0.0.1 or 0.1.0) on crates.io. The documentation looks solid, and the README explains its purpose well.

For the implementation, I like it, but I keep wondering if there could be something simpler. You mention having CollectDispatch2, CollectDispatch3, etc. traits. What if you could replace these all with a single trait, and instead have the number of different options as a generic parameter?

Something like

trait CollectDispatch<Res: Default> {
    fn dispatch_result(self, container: &mut Res);
}

which could be implemented like

impl<T, E, U, V> CollectDispatch<(U, V)> for Result<T, E>
where
    U: Container<T>,
    V: Container<E>,
{
    fn collect_dispatch(self, container: &mut (U, V)) {
        match self {
            Ok(v) => container.0.add_element(v),
            Err(e) => container.1.add_element(e),
        }
    }
}

It wouldn't have to be exactly that, of course. But if you wrote something similar, I think you could avoid the need for multiple traits - and instead just use the same trait generic over different tuples. What would you think of that?

Edit: I thought of a possible alternative crate. It seems like frunk might offer something like this? To be honest, I have no idea - frunk is something of an enigma to me. It just seems like the kind of thing that might be in there...

1 Like
#3

Hello,
First, thank you for your very constructive review. I do appreciate it.

About adding a new proc_macro
I have to say I am not a huge fan of proc_macro. I expected this library to be one of these no-dependencies crates that can easily be integrated and adopted. Adding a proc_macro would require the users to compile syn and quote (perhaps others? I have no experience with proc_macros), dramatically increasing the compile time.

On the other hand, as you pointed out, it removes a lot of repetitions, which would simplify its adoption. Additionaly, syn and quote are already used by a lot of other crates.

Consequently, I think the best option would be to make the proc_macro an opt-out option. This would allow new users to immediately take benefits from the crate, while advanced users who are looking for faster compile times would be able to reach their goal.

About replacing Container
This is a very nice idea. This will also simplify the API, making it simpler to understand.

About merging CollectDispatch*s
This one is very clever too. I was not comfortable with having so much traits defining roughly the same behavior.

Further thoughts
I was not comfortable with publishing this crate on crates.io before having feedback from the community. I think I will publish it by the end of the week. I will add a note in the README saying that the addition of a proc_macro is planned.

Well, frunk is an enigma to me too. Although I has no enum dispatching feature, I wouldn't be surprised if it allowed it in an unexpected way.

Again, thank you for your comments.

2 Likes