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

Call rescue_from with a method instead of a block #450

Merged
merged 4 commits into from
Aug 6, 2013
Merged

Call rescue_from with a method instead of a block #450

merged 4 commits into from
Aug 6, 2013

Conversation

robertopedroso
Copy link
Contributor

This PR adds a simple DSL for passing methods to rescue_from, much like Rails permits:

rescue_from Sequel::NoMatchingRow, with: rescue_missing_record

def rescue_missing_record
  Rack::Response.new('Missing Record', 404)
end

This means you can implement and call custom methods in your rescue handlers. At the moment, this is impossible because the passed block is executed via an instance_exec in the context of Grape::Middleware::Error.

The only problem is that this implementation cannot incorporate exception messages into the rescue method. Rails overcomes this limitation by passing the method as a symbol. It then calls the method with something like:

handler = method(rescuer)
handler.arity != 0 ? handler.call(exception) : handler.call

I'm not quite sure what changes would have to be made to Grape::Middleware::Error to enable this kind of functionality, but I'm hoping this PR is at least a start.

@dblock
Copy link
Member

dblock commented Aug 5, 2013

I like it. We do allow methods elsewhere, like for error_formatter and the argument is not a hash, but just a proc. I like the with syntax, but I think we should be consistent and just check whether the argument is a lambda, not a hash with a with option.

I would be open to support with if you want to make this change everywhere and make it backwards compatible, btw.

Also, please update the CHANGELOG.

Thanks!

@robertopedroso
Copy link
Contributor Author

I'd be happy to make the changes either way, but I'd just like to clarify what you mean. You're saying I should either make the syntax of rescue_from consistent with error_formatter, like this:

rescue_from ArgumentError, lambda { rescue_missing_record }

Or instead, that I change error_formatter (in a backwards-compatible way) to take a 'with' option? Like so:

# New format, takes an optional hash to assign a method or class
error_formatter :custom, with: CustomFormatter

# But retains the old functionality
error_formatter :custom, CustomFormatter
error_formatter :custom, lambda { ... }

Have I understood you correctly?

@dblock
Copy link
Member

dblock commented Aug 5, 2013

Correct. For this PR, start by doing the first part (make the syntax of rescue_from consistent with error_formatter. Make another PR that extends both to support with:.

@robertopedroso
Copy link
Contributor Author

Okay -- I reverted the first commit and made rescue_from consistent with error_formatter. I'll submit a PR for with support later tonight or tomorrow morning.

@dblock dblock merged commit 86f9853 into ruby-grape:master Aug 6, 2013
@dblock
Copy link
Member

dblock commented Aug 6, 2013

Merged, thank you very much.

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