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

Aggressive use of panic!() #154

Closed
teotwaki opened this issue Mar 5, 2022 · 1 comment · Fixed by #156
Closed

Aggressive use of panic!() #154

teotwaki opened this issue Mar 5, 2022 · 1 comment · Fixed by #156

Comments

@teotwaki
Copy link
Contributor

teotwaki commented Mar 5, 2022

Hi,

Thanks a lot for this library. Very nice!

From the documentation for TwitchIRCClient::join:

This method panics if a channel login of invalid format is passed to it. (As determined
by [crate::validate::validate_login].) If you are dealing with unsanitized
user input, you must manually call this validate function before calling this function,
in order to avoid panicking your program.

Would you be open to changing the signature of these functions to return something like a Result<(), ValidationError> instead of panicking? A few reasons for my request:

  • I don't want libraries that I use to call panic!() for trivial reasons.
  • panic!() should only be used when something goes really wrong. Attempting to join an ill-formed channel should not be that.
  • There is a function allowing you to verify whether the channel is apparently OK or not. So why panic at all?

I'm happy to implement the change and provide a PR if you're OK with it.

Thanks

@teotwaki teotwaki changed the title Unnecessary duplicate checks Aggressive use of panic!() Mar 5, 2022
@RAnders00
Copy link
Collaborator

I had a bit of a problem with returning Result<(), ValidationError> from those methods because it makes the API interface a bit clunky (requiring String is already a bit clunky...). I strive to make the API of the library sensible, simple to use and self-explanatory.

But you make a good point, I think I will reconsider this. I'll take the time today to change this.

@RAnders00 RAnders00 linked a pull request Mar 9, 2022 that will close this issue
1 task
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 a pull request may close this issue.

2 participants