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

Only catch formatter errors in formatter middleware #378

Merged
merged 5 commits into from
Apr 3, 2013

Conversation

kbarrette
Copy link
Contributor

Should fix #359

@tucker250
Copy link

+1

@dblock
Copy link
Member

dblock commented Apr 1, 2013

This definitely needs a spec to be merged, please.

@kbarrette
Copy link
Contributor Author

@dblock definitely - added specs.

@dblock
Copy link
Member

dblock commented Apr 1, 2013

Sorry, forgot to ask you to please update CHANGELOG as well. Sorry to be a pest.

@kbarrette
Copy link
Contributor Author

Done.

@kbarrette
Copy link
Contributor Author

@dblock Any update on this?

@dblock
Copy link
Member

dblock commented Apr 3, 2013

It's still on my todo list to merge. I wanted to add a spec to see what happens when the formatter raises some other exception and there's a rescue for :all. It should properly get caught and all that. Since this is a problem in my head I didn't want to ask you to add it, but that would definitely save me the time :)

@kbarrette
Copy link
Contributor Author

I'm happy to help, but I'm not sure I understand what you're proposing. My second example 'does not rescue other exceptions' should test the case where the formatter raises an exception that is not Grape::Exceptions::InvalidFormatter. I'm not sure where the rescue :all would be, since my thought here is that Grape should explicitly NOT rescue other exceptions.

@dblock
Copy link
Member

dblock commented Apr 3, 2013

Yes, yes, your spec is correct. I mean adding another spec that makes sure that if I do a rescue_from :all, then I catch any other exception from the formatter. Otherwise anything that goes wrong in the formatter cannot be rescued and so there's no way for a Rack API with Grape to return a 500 for example with a stack trace on "all other errors".

@kbarrette
Copy link
Contributor Author

Ah, I understand now! I'll add a spec shortly.

@kbarrette
Copy link
Contributor Author

Done.

@dblock
Copy link
Member

dblock commented Apr 3, 2013

@kbarrette Perfect, merging.

dblock added a commit that referenced this pull request Apr 3, 2013
Only catch formatter errors in formatter middleware
@dblock dblock merged commit 1fbeb7e into ruby-grape:master Apr 3, 2013
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.

Exceptions thrown in the formatter should be catchable by rescue_from :all
3 participants