CLN: Improve Exceptions in core/frame and testing in test_frame and test_multilievel #4732
Conversation
|
This is not a huge change - but anyone want to look over it? |
|
looks ok to me the data conflic error in update is odd |
|
It is weird. I wanted it to inherit from |
|
prob just something vestigial - just change to valueError (which I think u did)? |
|
+1 for |
|
nowhere. All three uses are: the definition of the exception, in update and in the test cases :P. I'm happy to remove it (right now it's just a subclass of ValueError). |
|
out of there |
|
cut---it---out ✂️ |
|
okie dokie, goodbye |
| raise ValueError('axis must be 0 or 1') | ||
| if axis not in (0, 1): # pragma: no cover | ||
| raise AssertionError('axis must be 0 or 1') | ||
| agg_axis = (axis + 1) % 2 |
There was a problem hiding this comment.
or 1 - axis if you like :) (doesn't matter)
There was a problem hiding this comment.
I like 1 - axis better - seems clearer to me.
|
(raise_with_traceback is pretty awesome. as is this pr) |
|
@hayd thanks! 😄 and thanks for catching those mysterious test removals! I spent some time removing obvious dups but definitely didn't want to remove things arbitrarily! |
|
@hayd I think it's good at this point, if you want to take a final look... |
| @@ -4392,7 +4395,7 @@ def _get_agg_axis(self, axis_num): | |||
| elif axis_num == 1: | |||
| return self.index | |||
| else: | |||
| raise Exception('Must have 0<= axis <= 1') | |||
| raise ValueError('Must have 0 <= axis <= 1') | |||
There was a problem hiding this comment.
to match others think should this be AssertionError('Axis must be 0 or 1. Got %s' % str(axis)) ?
There was a problem hiding this comment.
This error can bubble out to the user, which is why it's a ValueError, the others call a function that should only ever generate 0 or 1 (or error out), so it's a ValueErrror (that's why those other errors also have # pragma: no cover, they're basically just a catch that helper functions worked.)
There was a problem hiding this comment.
ah, I see, so you don't have AssertionErrors to be user facing?
But why not have the better message? (axis=0.5 is no good!)
There was a problem hiding this comment.
my impression is that usually you use an AssertionError to validate internal logic (this internal function will always be called with a list...etc.). (and that's why if you run something with -O it removes assertion errors - they are supposedly only checks for the programmer). That's a good point on the message, I'll change it.
There was a problem hiding this comment.
I thought that raise AssertionError will still raise with -o.
There was a problem hiding this comment.
it will..just generally, that's why asserts are stripped out with -O. [and for the specific cases here, it might actually be 'more' correct to have them be asserts, but it doesn't really matter]
|
Pretty tricky to spot if you've any other missing, but if you've checked then I'm happy. Also, re raise_with_traceback please consider answering this SO question: http://stackoverflow.com/questions/17001473/uncatching-an-exception-in-python :) |
* Adds `raise_with_traceback` to `pandas.compat`, which handles the Python 2/3 syntax differences for raising with traceback. * Uses `raise_with_traceback` in `assertRaises` and `assertRaisesRegexp` so they provide better feedback. ENH: raise_with_traceback method.
CLN: Big simplification of test_frame TST: Add test cases for bad input, etc. CLN: Exceptions that shouldn't happen should be assertions (mostly _apply*) CLN/TST: Right Exceptions + remove duplicate tests A few tests were duplicating the same thing or actually just the same test, those have been refactored or removed. Many tests were changed to specify the Exception they were looking for (and also to use the new `with_statement` format for assertRaises, etc.) CLN: Harmonize SparseDF and DF Exceptions ENH: SparseDF apply now accepts axis='columns'
CLN: Improve Exceptions in core/frame and testing in test_frame and test_multilievel
More specific Exceptions (and better testing for them) through a number of parts of pandas. In particular, in
core/frame/ tests/test_frame and tests/test_multilevel Associated changes in some other parts of pandas to support it (mostly making sparse have the same exceptions as core/frame).TypeErrornow checks message (to avoidTypeErrorconfusion from changes in signature).assertRaisesandassertRaisesRegexpas context managers.ReshapeError(which wasn't really being used anywhere)# pragma: no coverto hopefully make it easier to use coverage.raise_with_tracebackmethod incompatwhich allows for 2/3 compatible preservation of traceback.Obviously some squashing to come.
Related to #3954.