Skip to content
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

SaturnFromTitan
Copy link
Contributor

@SaturnFromTitan SaturnFromTitan commented Oct 19, 2019

Part of #22550

  • Replaced TestData usage in pandas/tests/series/test_rank.py with fixtures
  • Switched from class-based test approach to function-based (Note: This is why the diff is so big)
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

@simonjayhawkins
Copy link
Member

  • Switched from class-based test approach to function-based (Note: This is why the diff is so big)

Thanks for the PR. in future avoid this in the same PR as making changes. can always be done in a follow-up.

Copy link
Member

@simonjayhawkins simonjayhawkins left a 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

pandas/tests/series/test_rank.py Outdated Show resolved Hide resolved
pandas/tests/series/test_rank.py Outdated Show resolved Hide resolved
pandas/tests/series/test_rank.py Outdated Show resolved Hide resolved
@simonjayhawkins simonjayhawkins added Clean Testing pandas testing functions or related to the test suite labels Oct 19, 2019
@SaturnFromTitan SaturnFromTitan force-pushed the remove-testdata-in-series-tests_test_rank branch from 7c3d064 to 72512d5 Compare October 19, 2019 18:06
@SaturnFromTitan
Copy link
Contributor Author

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?

@simonjayhawkins
Copy link
Member

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.

@SaturnFromTitan
Copy link
Contributor Author

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)?
After this one, there are 5 more files pending

@SaturnFromTitan SaturnFromTitan force-pushed the remove-testdata-in-series-tests_test_rank branch 2 times, most recently from e1b4496 to fda2cf2 Compare October 21, 2019 19:32
@SaturnFromTitan
Copy link
Contributor Author

@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!

@simonjayhawkins
Copy link
Member

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)?
After this one, there are 5 more files pending

creating one PR per module is fine.

* Replaced TestData usage in pandas/tests/series/test_rank.py with
  fixtures
@SaturnFromTitan SaturnFromTitan force-pushed the remove-testdata-in-series-tests_test_rank branch from ea21d39 to 796f689 Compare October 22, 2019 14:28
@SaturnFromTitan
Copy link
Contributor Author

@jreback Can this one be merged as well? I took care of all review comments

@jreback jreback added this to the 1.0 milestone Oct 24, 2019
@jreback jreback merged commit e3698dc into pandas-dev:master Oct 24, 2019
@jreback
Copy link
Contributor

jreback commented Oct 24, 2019

thanks @SaturnFromTitan

HawkinsBA pushed a commit to HawkinsBA/pandas that referenced this pull request Oct 29, 2019
Reksbril pushed a commit to Reksbril/pandas that referenced this pull request Nov 18, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
bongolegend pushed a commit to bongolegend/pandas that referenced this pull request Jan 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants