-
Notifications
You must be signed in to change notification settings - Fork 213
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
Also use try/catch in Controller::API::V1::Comment #6262
Conversation
The txn_guard needs an explicit undef in catch Related progress issue: https://progress.opensuse.org/issues/176862
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 really an improvement.
try { $comment->handle_special_contents($self) } | ||
catch ($e) { | ||
undef $txn_guard; | ||
return $self->render(json => {error => $e}, status => 400); | ||
} |
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.
try { $comment->handle_special_contents($self) } | |
catch ($e) { | |
undef $txn_guard; | |
return $self->render(json => {error => $e}, status => 400); | |
} | |
eval { $comment->handle_special_contents($self) }; # using eval as the txn guard interacts badly with try-catch | |
return $self->render(json => {error => $@}, status => 400) if $@; |
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 advantage would be that we can filter for
$@
usage, only leavingeval
where we are not using the error message.
Let me demonstrate that in
how do you like that?
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 case where we need to undefine the txn guard is still worse.
I also don't see it as necessity to enforce use of try-catch and thus filtering doesn't seem very useful. As long as we don't use Try::Tiny
anymore which has counter-intuitive behavior for everyone coming from different programming languages we're good.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6262 +/- ##
=======================================
Coverage 99.01% 99.01%
=======================================
Files 398 398
Lines 40014 40020 +6
=======================================
+ Hits 39620 39626 +6
Misses 394 394 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 advantage would be that we can filter for $@
usage, only leaving eval
where we are not using the error message.
This pull request is now in conflicts. Could you fix it? 🙏 |
included with #6303 |
The txn_guard needs an explicit undef in catch
Related progress issue: https://progress.opensuse.org/issues/176862