-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Return error instead of panic from commands #3568
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
Conversation
Instead of panic in few commands, we can return an error to avoid unexpected panics in application code.
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
|
There is also ClientSetInfo command, but I was not sure if returning a error instead will mess up existing app logic using the pipelines. Happy to to take discuss on how to handle it. |
|
Hello @dragneelfps and thank you for your contribution! This was on our calendar for a while, glad you were able to address it. As for |
|
@ndyakov Honestly, I would prefer if go-redis can avoid panics altogether. Most clients usages don't handle panic well in my experience. Also, client initialization doesn't always need to be done in the initial app startup. For example, there could be scenarios where client is initialized dynamically(maybe on API request in case of server). There most servers don't expect and handle panics. Moreover, the ClientSetInfo already returns a response(StatusCmd) which can embed errors and I would assume the users will expect error there only, not a panic in case they provided invalid configuration or something. In short, as a user of the library, I would prefer an error in the response, instead of panic even for ClientSetInfo. By the way, this PR doesn't include any changes to ClientSetInfo so we can further discuss on it separately and have a future PR handle it. What do you think? |
|
@dragneelfps yes, let's merge this PR and we can continue a discussion in an issue, feel free to open one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, commands should not panic
Instead of panic in few commands, we can return an error to avoid unexpected panics in application code.
Instead of panic in few commands, we can return an error to avoid unexpected panics in application code.
Instead of panic in few commands, we can return an error to avoid unexpected panics in application code.
Resolves #2834