Allow rescue from non-StandardError exceptions to use default error handling

[Jelkster]

This continues to address issues discussed in #1713 and #1745.

@dm1try recently added the ability to explicitly rescue non-StandardError exceptions but I found that his solution required a block to be provided in order to handle exceptions or else it would raise an error due to no handler being found. This PR corrects that by re-using the existing find_handler and rescuable? logic, with some minor modifications to account for Exception being rescued instead of StandardError. This meant that the additional rescue Exception block could go away completely.

Edit: I forgot that @dm1try recommended we deprecate the use of :all so this additional option (rescue_from :exception - removed from PR after further discussion) is probably not necessary, as you can simply rescue_from Exception. The same as you can rescue_from StandardError for rescue_from :all.

[dm1try]

👍the code looks good to me, thank you!

as regards to the new :exception feature, I've never had to rescue from Exception 😎 so I have no strong feelings about this.

@dblock your turn

[dm1try]

we deprecate the use of :all so this additional option is probably not necessary, as you can simply rescue_from Exception. The same as you can rescue_from StandardError for rescue_from :all

yep, all adds ambiguity (as I mentioned in related issue) but it's so "sugar")
my point is that a user should not waste time to resolve this using README as he could just use his Ruby knowledge.

[dblock]

This change should not be required, the integration spec is specifically for mutli_xml, so it should never be in that else.

[Jelkster]

That makes sense about not ever being in the else since this is for multi_xml so I'll definitely remove that, but if I don't check for the presence of the constant, this is the only spec that fails for me locally on my ubuntu VM.

Failures:

  1) ActiveSupport::XmlMini uses multi_xml
     Failure/Error: expect(Grape::Xml).to eq(::MultiXml)

     NameError:
       uninitialized constant MultiXml
     # ./spec/integration/multi_xml/xml_spec.rb:6:in `block (2 levels) in <top (required)>'

Finished in 0.00072 seconds (files took 1.34 seconds to load)
1 example, 1 failure

Ref: https://github.com/ruby-grape/grape/blob/master/lib/grape/util/xml.rb

[dblock]

Run specs with rake, I think we cleverly exclude those as needed and run them with their own bundle?

[Jelkster]

Ahh, ok, I was running bundle exec rspec 🤦‍♂️

[dblock]

Stepping back I think :all is bad all around. I am down with deprecating it completely and forcing users to do rescue_from Exception or rescue_from StandardError with a major version bump. That can be done in a separate PR.

Therefore, before we add rescue_from :exception, are you are you saying @Jelkster that we cannot make a non-block version without introducing a symbol sugar? Otherwise I think we just want to fix that ability instead of adding :exception support.

[Jelkster]

@dblock, no, this code can handle non-blocks no problem without the symbol sugar. I was in the middle of implementing rescue_from :exception before @dm1try addressed the issue, so I just left it in there in case you wanted to take a look at the end result. It seems like we are all realizing the lack of need (and clarity) for the sugar.

[dblock]

So should we just close this and focus on deprecating :all? Maybe we still want README updates?

[Jelkster]

This PR actually added the ability to handle non-blocks (this test fails without it), so I think we'll still want that piece. I can convert this into a fix instead of a feature.

[dblock]

Makes sense, looking forward to an update @Jelkster. Thanks for hanging in there!

[Jelkster]

@dblock updated!

[dblock]

Great work @Jelkster. Want to tackle the deprecation of :all next?

[Jelkster]

I'm actually on vacation this week, but I can definitely take a stab when it's over.

[dblock]

What do we pay you for @Jelkster :)