-
-
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
adding option to allow grape to handle Grape::Exception when rescue all is set #1398
adding option to allow grape to handle Grape::Exception when rescue all is set #1398
Conversation
4ca918a
to
6f0cffe
Compare
@@ -1,7 +1,7 @@ | |||
0.16.3 (Next) | |||
============= | |||
|
|||
* Your contribution here. |
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.
Put this back please.
Would you please explain a bit more in detail what this achieves? I understand what it does, but I cannot think of a scenario where this is really useful. |
6f0cffe
to
2700b54
Compare
I added this so that I could easily respond with JSON errors and not HTML that leaked too much data and still have Grape handle 404's, 406's, and all the other exceptions that are handled out of the box. So, if you were to add Does that make sense? I can try to explain better if needed. |
There's still something odd about this feature. It skips a specific exception, handles the rest, so that seems very "special" to begin with. I want to talk it over more, I am sure we can come up with something better. Can't the same thing be accomplished with |
The exception
I just copied the same logic. I wanted to put this repeated logic into one place but couldn't think of a clean way to do it. I don't think it'll work the same if we do EDIT: I totally agree that the name could be better though. Any suggestions? |
Ok, but don't you think a DSL like |
yeah. I like that name better. it doesn't explicitly say that its also rescuing :all but a decently descriptive README might make up for it. I'll make that change and we can take a look at it. @dblock change completed. let me know what you think. |
fa98bcb
to
4b2411d
Compare
@dblock change completed. let me know what you think. |
@@ -3,6 +3,8 @@ | |||
|
|||
* Your contribution here. | |||
|
|||
* [#1398](https://github.com/ruby-grape/grape/pull/1398): Added rescue_from :grape_exceptions option to allow Grape to use the built in Grape::Exception handing and use rescue :all behavior for everything else. [@mmclead](https://github.com/mmclead) |
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.
Make this look like every other line please, the period goes to the end and there's a dash before your name ;)
Made some comments, it's mostly ready. Try to shorten the README, get to the point :) |
…ape::Exception handling
4b2411d
to
68717c6
Compare
@dblock updated again. Sorry my grammar is terrible. And, I answered comment on test. Your call on whether you want the full match or continue with the way other examples are. |
Merged, thank you. |
This will allow Grape to use the built in Grape Exceptions defined in /lib/grape/exceptions when rescue :all is set.