chore: Remove the need for explicit bubble up of certain exceptions#29235
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #29235 +/- ##
===========================================
+ Coverage 60.48% 83.72% +23.23%
===========================================
Files 1931 518 -1413
Lines 76236 37513 -38723
Branches 8568 0 -8568
===========================================
- Hits 46114 31408 -14706
+ Misses 28017 6105 -21912
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Right. I considered doing the same during #29166, but thought there might have some reason for the code to point out the "likely exception" in the code, maybe to allow future handling or something. I decided to leave them as they were since my goal was enabling the lint rule and minimizing the code changes. |
mistercrunch
left a comment
There was a problem hiding this comment.
Approving as those lines effectively do nothing - except maybe for calling out semantically in the code the exceptions that are likely to happen during the try block.
|
@mistercrunch the catalyst for the change is I was rebasing #24969 (a longstanding PR I've been wrangling with failing tests for a while). |
@mistercrunch I think for this case, the best approach is to document the exceptions in docstrings: |
2c1f467 to
be05ada
Compare
|
@michael-s-molina I addressed your comment. |
SUMMARY
There are instances where we bubble up exceptions, however this is akin to simply not catching it, since any uncaught exception is propagated up the call stack.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION