-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added custom exceptions to Grape. Updated validation errors to use ValidationError. #221
Conversation
…onError that can be rescued.
I think this is perfect. All you need is maybe to update the README with some notes on rescuing validation errors? Will leave this open for a bit for comment. |
I'll do one big README update after we get some feedback on this. |
raise unless options[:rescue_all] || (options[:rescued_errors] || []).include?(e.class) | ||
handler = options[:rescue_handlers][e.class] || options[:rescue_handlers][:all] | ||
is_rescuable = rescuable?(e.class) | ||
if e.is_a?(Grape::Exceptions::Base) && !is_rescuable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why woudln't rescuable?
return true for a Grape::Exception::Base
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this code could be much simpler then:
if rescuable?(e.class)
handler =
else
raise
handler = ...
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure I messed up this condition here, but you get the idea :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rescuable? checks if you have any rescue handlers set for an exception. It would evaluate to true if you had explicitly setup a rescue handler for Base (which you don't want to do, since it falls back on the default handler). I'll think a little bit about how to make this clearer.
Will also find some time in the next few days to address some old comments from other pull requests and updating the documentation.
Merging this. Please feel free to make further improvements on my minor comments. |
Added custom exceptions to Grape. Updated validation errors to use ValidationError.
Looks like this broke the build for 1.9.2 and 1.9.3, please take a look. |
Taking a look now. For future reference, is there a good way I can test against multiple environments? |
@adamgotterer Set up your fork to build on Travis-ci. |
That was easy. Thanks! |
Custom exceptions should extend Grape:: Exceptions::Base. The current error/exception handler will treat the base class the same way that it treats :error. Anything extending Base can be rescued and acts like a more traditional exception. Converted Validation errors to use ValidationError, which is now an exception available anywhere in Grape.
Would love to hear thoughts and suggestions if there is room for improvement.
Original comment thread and concept from #201.