-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
[#22550] Remove TestData from series-tests test_rank.py #29101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#22550] Remove TestData from series-tests test_rank.py #29101
Conversation
Thanks for the PR. in future avoid this in the same PR as making changes. can always be done in a follow-up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SaturnFromTitan Thanks for the PR. generally looks good. a couple of suggestions
7c3d064
to
72512d5
Compare
Thanks for your review @simonjayhawkins! I took care of your first two comments. Do you want me to create a new issue for the other comment you made about parametrisation? |
see what @jreback has to say. if the switch from class-based test approach to function-based is done as a follow-up instead of here, the additional fixtures won't be necessary here for the task of removing the TestData base class. in this case the parameterisation of the three tests that use the class variables could be done as a precursor to the switch. |
Gotcha. Just let me know if I should revert the class to function switch - it won't be a lot of effort and I wouldn't mind. On another note: I'll probably also take care of replacing TestData for the other series modules over the coming days. Should I continue creating one PR per module or is it fine to group the remaining ones in a single PR (possibly doing 1 commit per file)? |
e1b4496
to
fda2cf2
Compare
@simonjayhawkins @jreback I undid the class- to function-based transition to separate concerns and aid the review process. Would be great if one of you could also reply to my previous comment. Cheers! |
creating one PR per module is fine. |
fda2cf2
to
ea21d39
Compare
* Replaced TestData usage in pandas/tests/series/test_rank.py with fixtures
ea21d39
to
796f689
Compare
@jreback Can this one be merged as well? I took care of all review comments |
thanks @SaturnFromTitan |
Part of #22550
pandas/tests/series/test_rank.py
with fixturesblack pandas
git diff upstream/master -u -- "*.py" | flake8 --diff