-
Notifications
You must be signed in to change notification settings - Fork 576
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
Add exceptions to stash on all exception formats #2061
Conversation
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.
The tests do nothing.
@kraih Can you elaborate a little bit? The way I see it is the exception will become undef if there is no exception in the stash via the after dispatch hook. Should I be using |
You can literally run those tests without the proposed change and everything passes. |
@kraih Okay, I think I got it, sorry, I see now. |
Don't forget to squash commits. |
bfcb1a5
to
289da9b
Compare
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'm positive to this change. I've often overridden the exception helper to achieve the same, which I think I can stop doing after this 👍
Wouldn't mind if the _is_e() method was inlined though.
@@ -90,6 +90,11 @@ sub _content { | |||
return Mojo::ByteStream->new($hash->{$name} // ''); | |||
} | |||
|
|||
sub _convert_to_exception { | |||
my $e = shift; | |||
return _is_e($e) ? $e : Mojo::Exception->new($e); |
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.
Not sure if this is very important, but could _is_e()
be inlined in this function? Looks like it's only being used one place now.
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'll make that change right away, thanks for the feedback!
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.
Did _is_e() come back by accident?
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.
Must have screwed up my commit whoops
2292cd4
to
45f38d9
Compare
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.
LGTM 👍
One thing I think should be considered is if we want to render Edit: I've changed it to |
45f38d9
to
54083e7
Compare
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.
LGTM :)
54083e7
to
addf3ca
Compare
Umm, I'm not sure what happened but this was closed? |
Okay I'm an idiot, re-opening this in a different PR, I guess I accidentally changed a commit signature in my rebase :L |
Summary
Make it easier to use exceptions, when exception format is not HTML
Motivation
It doesn't make sense for exceptions to be stored in the stash only if the format is HTML, instead it makes more sense, and is more consistent to store them in the stash for all formats
References
fixes #2048