rubocop / rails-style-guide

A community-driven Ruby on Rails style guide
http://rails.rubystyle.guide
6.47k stars 1.06k forks source link

avoiding the use of the StandardError exception #331

Open MaximilianoGarciaRoe opened 1 year ago

MaximilianoGarciaRoe commented 1 year ago

In general, it is better to rescue and display the specific error rather than showing a generic message in the view when working with Ruby. This is because a generic message may not provide enough information for the user to understand the problem and take steps to solve it.

By rescuing and displaying the specific error, more detailed information about the nature of the problem can be provided, which can help the user understand what went wrong and how to fix it.

I hope you consider this PR please, it would be my first contribution.

andyw8 commented 1 year ago

I agree with the premise, but I wonder if it's too general. In any language, it's usually wise to avoid directly exposing users to errors in the underlying framework.

MaximilianoGarciaRoe commented 1 year ago

I agree with the premise, but I wonder if it's too general. In any language, it's usually wise to avoid directly exposing users to errors in the underlying framework.

Yes, but the explicit warning is not present in the code, StandarError. In the specific case of Rails, it would be good to have this warning explicitly included to promote this good practice.

MaximilianoGarciaRoe commented 1 year ago

Hi @andyw8 Could you help me understand how you would suggest implementing this change? Would you suggest making the change in a different location, or perhaps adding more explanation to the proposal?"

pirj commented 1 year ago

What is wrong with the following approach? What I'm trying to do is to take the focus off StandardError class itself.

begin
  raise StandardError.new("No access")
rescue => e
  @message = e.message
  render :error
end

There certainly are cases when advanced handling is needed, and there's even a whole Exceptional Ruby book about this. But what is the problem for the basic case?

MaximilianoGarciaRoe commented 1 year ago

What is wrong with the following approach? What I'm trying to do is to take the focus off StandardError class itself.

begin
raise StandardError.new("No access")
rescue => e
@message = e.message
render :error
end

There certainly are cases when advanced handling is needed, and there's even a whole Exceptional Ruby book about this. But what is the problem for the basic case?

In this particular case, the current implementation of the code is appropriate. However, in more complex applications or broader contexts, it is important to consider which parts of the code would benefit from specific error handling. That's why I suggested adding it to the Controllers, where vulnerabilities are more likely. What do you think? Do you agree with this proposal or do you have another suggestion for where to implement this change?

@pirj