TST: reintroduce check_series_type in assert_series_equal #32670
Conversation
|
I think worth completely reverting rather than part of the original PR |
WillAyd
added
Testing
Why is that? #32513 included some non-downstreaming-breaking improvements |
|
Yes, just adding back only this is perfect IMO. The rest of the PR was actually completely unrelated to this change (regardless of the title of the PR that seemed to indicate otherwise). |
|
We might want to add a test though |
|
Added a test |
| tm.assert_series_equal(s1, s3, check_series_type=False) | ||
|
|
||
| with pytest.raises(AssertionError, match="Series type not equal"): | ||
| tm.assert_series_equal(s1, s3, check_series_type=True) |
There was a problem hiding this comment.
should this test include tm.assert_series_equal(s3, s1, check_series_type=True) (with an xfail))
There was a problem hiding this comment.
It should but the original check_series_type should also be extended as
isinstance(s3, type(s1)) is True while isinstance(s1, type(s3)) is False. assert_series_equal should probably return the same result no matter the order of left and right Series.
There was a problem hiding this comment.
yeah, I think the fix is outside of the scope of this PR since this is simply reverting changes. but if we are adding a new test, the test should probably test all permutations for completeness. Maybe add a TODO comment in the test instead.
There was a problem hiding this comment.
I think we can just fix it here at once?
There was a problem hiding this comment.
could maybe remove the comments in assert_series_equal now?
There was a problem hiding this comment.
I think we can just fix it here at once?
@jorisvandenbossche that broke (some) tests.
There was a problem hiding this comment.
Ah, should be easy to fix the test to make proper use of assert_frame_equal, I think (will look at it tomorrow)
|
@simonjayhawkins this is passing now. Are you OK with including the fix here? |
Ah, yes, forgot that. That actually simplified it a lot |
|
lgtm |
…#32670) Co-authored-by: Joris Van den Bossche <[email protected]>
black pandasgit diff upstream/master -u -- "*.py" | flake8 --diffRe-introduced
check_series_typeinassert_series_equalwhich was recently removed in #32513. It was a part of public API which is not used internally, but it is used elsewhere (like geopandas). Discussion in #32513 suggests that it should be put back where it was.