Removed raise_from_traceback #29174
Removed raise_from_traceback #29174
Conversation
WillAyd
added
2/3 Compat
Error Reporting
pandas/io/html.py
Outdated
| @@ -912,11 +909,9 @@ def _parse(flavor, io, match, attrs, encoding, displayed_only, **kwargs): | |||
| "different flavor.".format(flav) | |||
| ) | |||
|
|
|||
| retained = caught | |||
| raise | |||
There was a problem hiding this comment.
does this change the current behavior? for/else is a pretty obscure idiom that i dont have intuition for
There was a problem hiding this comment.
Also if you're inclined to tighten the except Exception above while in the neighborhood, that'd be cool
There was a problem hiding this comment.
The else clause in for...else would only get executed if there was no break in the for block. In this cause it would break if no exception was raised in the try...else
Come to think of it we could get rid of the try...else now too. I'll see what that looks like after this CI run completes (mostly want to see if it helps with the HTML issue visibility)
There was a problem hiding this comment.
Actually misread that. This goes through a fallback pattern of the various parsers trying one after the next. I erroneously just raised on first available - might not be well tested...
For now just reverted; can open a follow up for clean up and except Exception handling as you mentioned
|
Looks like this does actually preserve the traceback: https://travis-ci.org/pandas-dev/pandas/jobs/601566770#L2267 |
pandas/util/testing.py
Outdated
| @@ -2533,7 +2533,7 @@ def exception_matches(self, exc_type, exc_value, trace_back): | |||
| pat=self.regexp.pattern, val=val | |||
| ) | |||
| e = AssertionError(msg) | |||
| raise_with_traceback(e, trace_back) | |||
| raise e.with_traceback(trace_back) | |||
There was a problem hiding this comment.
since assert_raises_regex has been deprecated internally, can we just remove the complete _AssertRaisesContextmanager class.
There was a problem hiding this comment.
Not overly familiar with this code but seems very logical...I'll give it a shot and see what happens
There was a problem hiding this comment.
Hmm well thinking this through some more some of our pandas.util.testing items have leaked into the public API, so we might want to deprecate first. I'll open an issue to discuss
There was a problem hiding this comment.
It was publicly deprecated in 0.24, so with the new version policy, I think ok to remove in 1.0
There was a problem hiding this comment.
Ah... missed that. Makes sense - thanks
|
unused sys import |
|
Comments addressed and all green here |
|
LGTM |
There was a problem hiding this comment.
Thanks @WillAyd I'll merge this as-is, but as a follow-up could you remove the code check and add the deprecation to whatsnew.
|
normally on a deprecation removal we should have the whatsnew note along with all of the changes in the same PR as otherwise it’s harder to track / follow |
|
Nice to see this removed as a result of our move to |
ref #29170
This is a pretty old compat function that I think actually hides the errors we are looking for when run via load scheduling. I've converted to a mixture of
raise...fromandraise (e).with_traceback()depending on what was available. Not sure it matters too much either wayLet's see...