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

Rescue truly all exceptions #1745

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

Conversation

mtsmfm
Copy link

@mtsmfm mtsmfm commented Mar 1, 2018

Fix #1713

Currently, rescue_from can't rescue from exceptions which aren't inherited from StandardError.
Due to this constraint, we can't rescue exceptions which are inherited from Exception.
Some exceptions are inherited from Exception because they should be rescued explicitly. (ex. Rack::Timeout::RequestTimeoutException)

Before v0.6.0, it can rescue truly all exceptions but unintentionally it changed because of rubocop.

I think grape should be able to rescue from all exceptions like Rails.

Perhaps it'll affect a lot but I think current users assume rescue_from :all can rescue from truly all exceptions.

What do you think?

@dblock
Copy link
Member

dblock commented Mar 1, 2018

Thanks. A few things need to happen to merge this change:

  • A backward compatible way to rescue only StandardError errors. 0.6.0 is very old, so maybe at that point we should really consider leaving the current default and just add support for rescue_from :exception?
  • UPGRADING.md
  • Major version bump

@mtsmfm
Copy link
Author

mtsmfm commented Mar 2, 2018

Thank you for the reviewing!

A backward compatible way to rescue only StandardError errors

I think rescue_from StandardError is the way

@dm1try
Copy link
Member

dm1try commented Mar 10, 2018

I think current users assume rescue_from :all can rescue from truly all exceptions.

Actually, I always thought that rescue_from :all is a counterpart for standard ruby behavior when rescue is called without an explicit exception type; so using :all is just a DSL "compromise" for this behavior(.._from suffix needs some param 😬 )

# :(
rescue_from do
  ...
end

and read rescue_from :all like default_rescue)

Currently, rescue_from can't rescue from exceptions which aren't inherited from StandardError.

So I suggest fixing this first and add to README that rescue_from :all behaves like standard rescue.

Otherwise, the original intention of providing special exceptions like Rack::Timeout::RequestTimeoutException will be broken(the intention was to force a developer to handle such exceptions explicitly)

P.S. truly explicitness => deprecate :all at all :)

@dblock
Copy link
Member

dblock commented Mar 10, 2018

Makes sense @dm1try. Thanks. I'm still looking for a mergeable PR, see my comments above.

@dblock
Copy link
Member

dblock commented Mar 11, 2018

Rebase against #1749.

@dblock
Copy link
Member

dblock commented Apr 19, 2018

What should we do with this @mtsmfm?

@mtsmfm
Copy link
Author

mtsmfm commented Apr 19, 2018

Sorry for being late 🙇
Rebased and force pushed!

@dblock
Copy link
Member

dblock commented Jun 6, 2018

Just seeing this, apologies for the delay.

This is also a behavior change. We just merged #1765 which forced a major version bump with a backward incompatible change, so we should put this one in as well. @mtsmfm would you mind adding a paragraph to UPGRADING.md too please?

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.

None yet

3 participants