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

Fix rescue_from documentation #649

Closed
wants to merge 1 commit into from
Closed

Conversation

amacneil
Copy link

I'm not sure whether this is a bug or intentional, but right now rescue_from does not rescue subclasses by default.

This seems to be caused by https://github.com/intridea/grape/blob/master/lib/grape/api.rb#L217

handler_type = !!options[:rescue_subclasses] ? :rescue_handlers : :base_only_rescue_handlers

@dblock
Copy link
Member

dblock commented May 21, 2014

The documentation is certainly the intended behavior as discussed in #544. Are you sure that's not happening? Can you please write a test that reproduces the problem? /cc: @xevix

@xevix
Copy link
Contributor

xevix commented May 23, 2014

@dblock Confirmed. Adding a unit test to ensure the default is true, and fixing.

@xevix
Copy link
Contributor

xevix commented May 23, 2014

@adrianmacneil From what I can tell, handler_type is being assigned to the proper value, but somewhere deeper it's not being set properly. Looking.

@dblock
Copy link
Member

dblock commented May 23, 2014

Thanks @xevix for following up on this, looking forward to a PR.

@xevix
Copy link
Contributor

xevix commented May 23, 2014

@adrianmacneil I take that back. That's definitely the cause.

@dblock Working on one. Fixing this however breaks some other unit tests, so I'll work on fixing those first.

@amacneil
Copy link
Author

Hey, thanks for the pull request and fix :)

I wasn't sure whether this was intended behavior or an actual bug, but thanks for fixing so quickly!

@amacneil
Copy link
Author

Fixed by #651

@amacneil amacneil closed this May 26, 2014
@xevix
Copy link
Contributor

xevix commented May 27, 2014

@adrianmacneil Thanks for reporting!

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.

3 participants