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 callbacks Option<dyn FnMut> #27

Closed
wants to merge 2 commits into from

Conversation

cmleinz
Copy link
Contributor

@cmleinz cmleinz commented Jan 26, 2024

We can relax the trait bounds for callbacks to be FnMut rather than Fn, also can remove the Sync bound

@craftytrickster
Copy link
Owner

Removing sync probably isn't a big deal, but changing the types on the options is a breaking change.

If you are to do that, we should bump the minor version of the crate, and, at the same time, take advantage to implement - #23

@cmleinz
Copy link
Contributor Author

cmleinz commented Feb 3, 2024

Sounds good to me. If the on_disconnect callback gets an arg with the error what should the type be? Since we initiate the callback on Ok(0) as well as Err(err).

@craftytrickster
Copy link
Owner

After giving this another look, I am not entirely convinced it is worth adding additional information to the trying to connect message. What do you think?

on_connection failed probably should have Err and you could argue that on_disconnection could have an Optional Err.

As a user, do you think having those extra errors are actually valuable?

@craftytrickster
Copy link
Owner

I think for now, it isn't worth changing the interface, I prefer to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants