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.

Sign up for GitHub

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

Jump to bottom

Add exceptions to stash on all exception formats #2061

Closed
wants to merge 0 commits into from
Closed

Add exceptions to stash on all exception formats #2061

wants to merge 0 commits into from

Conversation

rawleyfowler
Copy link
Contributor

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

kraih
kraih previously requested changes Apr 23, 2023
View reviewed changes
Copy link
Member

@kraih kraih left a 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.

@rawleyfowler
Copy link
Contributor Author

@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 is or ok instead?

@kraih
Copy link
Member

kraih commented Apr 23, 2023

@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 is or ok instead?

You can literally run those tests without the proposed change and everything passes.

@mergify mergify bot dismissed kraih’s stale review April 23, 2023 23:43

Pull request has been modified.

@rawleyfowler
Copy link
Contributor Author

@kraih Okay, I think I got it, sorry, I see now.

@rawleyfowler rawleyfowler requested a review from kraih April 30, 2023 18:43
@kraih
Copy link
Member

kraih commented Apr 30, 2023

Don't forget to squash commits.

@kraih kraih requested review from a team, marcusramberg, jberger and Grinnz April 30, 2023 23:52
jhthorsen
jhthorsen previously approved these changes May 2, 2023
View reviewed changes
Copy link
Member

@jhthorsen jhthorsen left a 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.

lib/Mojolicious/Plugin/DefaultHelpers.pm Outdated
@@ -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);
Copy link
Member

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.

Copy link
Contributor Author

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!

Copy link
Member

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?

Copy link
Contributor Author

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

@rawleyfowler rawleyfowler force-pushed the stash-exceptions branch 2 times, most recently from 2292cd4 to 45f38d9 Compare May 3, 2023 02:50
jhthorsen
jhthorsen previously approved these changes May 3, 2023
View reviewed changes
Copy link
Member

@jhthorsen jhthorsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@rawleyfowler rawleyfowler requested a review from kraih May 4, 2023 01:18
@rawleyfowler
Copy link
Contributor Author

rawleyfowler commented May 4, 2023 •
edited

One thing I think should be considered is if we want to render $e or $e->inspect, seeing as today it simply renders the text. Any thoughts? In terms of line 203 and line 323

Edit: I've changed it to ->inspect because I believe that is the expected behavior.

marcusramberg
marcusramberg approved these changes May 5, 2023
View reviewed changes
Copy link
Member

@marcusramberg marcusramberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@kraih kraih requested a review from jhthorsen May 5, 2023 09:35
@rawleyfowler
Copy link
Contributor Author

Umm, I'm not sure what happened but this was closed?

@rawleyfowler
Copy link
Contributor Author

Okay I'm an idiot, re-opening this in a different PR, I guess I accidentally changed a commit signature in my rebase :L

@rawleyfowler rawleyfowler mentioned this pull request May 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcusramberg marcusramberg marcusramberg approved these changes

@jberger jberger Awaiting requested review from jberger jberger was automatically assigned from mojolicious/maintainers

@Grinnz Grinnz Awaiting requested review from Grinnz Grinnz was automatically assigned from mojolicious/maintainers

@kraih kraih Awaiting requested review from kraih

@jhthorsen jhthorsen Awaiting requested review from jhthorsen

Assignees
No one assigned
Labels
needs feedback
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exceptions are not available in the after_dispatch hook when exception_format is set to json
4 participants
@rawleyfowler @kraih @marcusramberg @jhthorsen