Skip to content

Suggestion : No rescue without class #2943

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

Closed
coding-red-panda opened this issue Mar 11, 2016 · 10 comments
Closed

Suggestion : No rescue without class #2943

coding-red-panda opened this issue Mar 11, 2016 · 10 comments

Comments

@coding-red-panda
Copy link

Hello,

just a small suggestion.
Would it be possible to create a cop that checks rescue statements and if they are actually catching a specific error?

This would be accepted:

def some_function
  begin
    do_something
  rescue StandardError => e
    do_something_else
  end
end

def some_function
  do_something
rescue StandardError => e
  do_something_else
end

And these 2 examples would not be accepted anymore:

def some_function
  begin
    do_something
  rescue
    do_something_else
  end
end

def some_function
  do_something
rescue
  do_something_else
end

It's kinda a policy we have in our team that specific errors should be caught and not just every globbed up.

@alexdowad
Copy link
Contributor

We already have something like this. Lint/RescueException flags rescue Exception, since it "swallows" all exceptions.

@coding-red-panda
Copy link
Author

does that cover simply writing rescue without a class? cause just rescue defaults to StandardError

@alexdowad
Copy link
Contributor

Maintainers, do we want something like this? There are a lot of Ruby codebases which use idioms like:

a = Integer(str) rescue nil

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 19, 2016

@alexdowad I think we have a cop that checks for this very same thing. :-) I think RescueException can be extended to handle @NekoNova's case as this is more or less the same thing.

@HParker
Copy link

HParker commented Nov 14, 2016

I looked into this briefly. From what I found the RescueException cop is there to prevent catching exceptions that you maybe shouldn't be catching. The suggestion in this issue is that rescue blocks should communicate more about what type of exception they are trying to catch.

To me that sounds like a different cop. Does that sound right to others as well?

@jonas054
Copy link
Collaborator

@HParker That sounds right. The message from Lint/RescueException is "Avoid rescuing the Exception class. Perhaps you meant to rescue StandardError?". Reporting where StandardError is caught without being explicitly named is a different issue. So, a new cop.

@Drenmi
Copy link
Collaborator

Drenmi commented Jan 28, 2017

@HParker: Agree with both of you on that. 😀

Drenmi added a commit to Drenmi/rubocop that referenced this issue Jun 30, 2017
This cop checks for cases where `rescue` is used without specifying one
or more errors to rescue, e.g.:

```
begin
  foo
rescue
  bar
end
```
@olegantonyan
Copy link

Why rescue StandardError is ok, but just rescue is not if these two are the same? I would expect either rescue StandardError the same offense, or not to consider rescue without class an offense at all. It's just a syntactic sugar in Ruby.

@coding-red-panda
Copy link
Author

it's about being explicit and forcing people to think about what they want to rescue instead of gobbling up all errors.
If you catch an exception, you should handle it and do something with it, not just gobble it up.

That was the main idea behind my feature request for this.

@Drenmi
Copy link
Collaborator

Drenmi commented Sep 19, 2017

The difference is saying "I'm deliberately rescuing StandardError, just so you know" vs. saying "I might have no idea what I'm rescuing, or I might, you figure it out." 🙂

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

No branches or pull requests

7 participants