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

Modify or to || and Exception to RuntimeError #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

avosa
Copy link

@avosa avosa commented May 7, 2022

1. Altered or with ||

Given this line:

if error_info['status'] != 'error' or not error_info['name']

We want to give higher precedence to error_info['status'] != 'error' and revert to check for not error_info['name'] if and only if the first check fails. This will help improve the speed of our program as well!

2. Altered Exception to RuntimeError

Given the code snippet:

 class Error < Exception

Lint/RescueException looks for misuse of Exception in a rescue statement, and dutifully suggests to rescue from StandardError which is Ruby's default for rescue. Since there is a high chance a similar mistake when declaring an exception class is being made, the suggested fix may lead to the exception never being caught.

As an example, picture someone following Rubocop's advice and fixes this:

class FooError < Exception; end

begin
  raise FooError
rescue Exception
  puts 'rescued'
end

into this:

class FooError < Exception; end

begin
  raise FooError
rescue StandardError
  puts 'rescued'
end

when he should have been doing this instead:

class FooError < RuntimeError; end

begin
  raise FooError
rescue StandardError
  puts 'rescued'
end

As a consequence it is quite interesting to lint for custom exceptions that may mistakenly inherit from Exception instead of either StandardError or RuntimeError, the latter being preferred since it's the default for fail/raise (perusing the symmetry with rescue being prodded into Ruby's default behaviour).

Expected behavior:

# no problem
class FooError; end

# no problem, if RuntimeError is preferred (default)
class FooError < RuntimeError; end

# no problem, if StandardError is preferred (alternate `EnforcedStyle`)
class FooError < StandardError; end

# this is a problem
class FooError < Exception; end

# should it be legit, it should be explicitly ignored by a directive along with due documentation
class ReallyAnException < Exception; end # rubocop:disable Lint/InheritException

Conclusion:
Avoid inheriting from the Exception class. Perhaps you meant to inherit from RuntimeError

@jethroo
Copy link
Collaborator

jethroo commented May 9, 2022

hi @avosa thanks for the PR and pointing out the issues. We forked the gem from its bitbucket origin in order to apply some tweaks needed for or journey towards upgrading to rails 7. So we do not actively maintain it it that sense.

Changing in the error class to the propriate one though is a public interface change of this gem so we have to think of if we want to support this change by now. Also to mention that we rather are moving away from Mandrill ;).

I keep you posted but can't promise anything by now.

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.

2 participants