CLN: unnecessary exception catching #29298
Conversation
jorisvandenbossche
left a comment
jorisvandenbossche
left a comment
There was a problem hiding this comment.
Just a comment about the adapted error message, the rest looks good
| raise | ||
| for i, v in enumerate(series_gen): | ||
| results[i] = self.f(v) | ||
| keys.append(v.name) |
There was a problem hiding this comment.
I personally think this can be informative, though.
Is this that problematic? (we are raising the same exception again, so it is not hiding any exceptions?)
There was a problem hiding this comment.
The index information can be useful, but this can also make it harder to reason about what parts of the code are exception-raising.
This can go either way. If there's a consensus to keep it, I'll revert.
There was a problem hiding this comment.
Can you explain how this makes it harder to reason about? This simply reraises what has been raised (with an edited error message), no?
There was a problem hiding this comment.
Can you explain how this makes it harder to reason about?
Well it's an idiosyncratic thing, so no.
jreback
left a comment
jreback
left a comment
There was a problem hiding this comment.
lgtm. I undestand @jorisvandenbossche concern but this seems not a big deal
WillAyd
left a comment
WillAyd
left a comment
There was a problem hiding this comment.
yea agree with everyone on this one :-)
5 participants
adding an index to the exception is a nice thought, but ultimately adds more complication than information.
The
except AssertionErrors were needed back when the lines below wereexcept Exception, but those have been made more specific, so these are no longer needed