Skip to content

CLN: Replace DataFrame() with empty_frame in tests #33167

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

CLN: Replace DataFrame() with empty_frame in tests #33167

wants to merge 7 commits into from

Conversation

jason3804
Copy link

@jason3804 jason3804 commented Mar 31, 2020 •
edited
Loading

  • closes Use empty_frame fixture #33161
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@pep8speaks
Copy link

pep8speaks commented Mar 31, 2020 •
edited
Loading

Hello @Jason-M-Chan! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-02 03:51:38 UTC

@jason3804 jason3804 changed the title Empty frame fixture CLN: Replace DataFrame() with empty_frame() in tests Mar 31, 2020
@ShaharNaveh
Copy link
Member

@Jason-M-Chan Thank you for taking on this!

My suggestion is to have smaller PRs, that way it will be easier to debug if necessary, and to avoid future merge conflicts.

@WillAyd WillAyd mentioned this pull request Mar 31, 2020
Closed
5 tasks
WillAyd
WillAyd requested changes Mar 31, 2020
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - looks pretty good!

WillAyd
WillAyd requested changes Mar 31, 2020
View reviewed changes
pandas/tests/frame/test_apply.py Outdated
@@ -1365,7 +1363,7 @@ def test_agg_cython_table(self, df, func, expected, axis):
"df, func, expected",
chain(
tm.get_cython_table_params(
DataFrame(), [("cumprod", DataFrame()), ("cumsum", DataFrame())]
empty_frame, [("cumprod", empty_frame), ("cumsum", empty_frame)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add the fixture to the function signature here

@WillAyd WillAyd added the Testing pandas testing functions or related to the test suite label Mar 31, 2020
@jason3804
Copy link
Author

I made the changes recommended above, but the Azure Pipeline still gives errors saying

TypeError: setup_method() missing 1 required positional argument: 'empty_frame'

I'm not sure how to debug this. Also, sorry about the large pull request, I will make smaller ones.

WillAyd
WillAyd requested changes Apr 1, 2020
View reviewed changes
pandas/tests/indexing/common.py Outdated
@@ -89,7 +89,7 @@ def setup_method(self, method):
self.series_ts_rev = Series(np.random.randn(4), index=dates_rev)
self.frame_ts_rev = DataFrame(np.random.randn(4, 4), index=dates_rev)

self.frame_empty = DataFrame()
self.frame_empty = empty_frame
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to pass empty_frame as a parameter to setup_method here; I think would fix the CI issue you are asking about

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the common.py file can't actually access fixtures. I reverted back to DataFrame() instead.

@jason3804 jason3804 requested a review from WillAyd April 2, 2020 02:48
@jason3804 jason3804 changed the title CLN: Replace DataFrame() with empty_frame() in tests CLN: Replace DataFrame() with empty_frame in tests Apr 2, 2020
@jbrockmendel
Copy link
Member

-1 on this. there are good use cases for fixtures, this is not one of them.

@jbrockmendel jbrockmendel reopened this Apr 2, 2020
@WillAyd
Copy link
Member

WillAyd commented Apr 7, 2020

@jbrockmendel can you clarify your objection? I think this helps make what we consider an empty_frame consistent across the code base

@jbrockmendel
Copy link
Member

can you clarify your objection? I think this helps make what we consider an empty_frame consistent across the code base

  1. as you mentioned in another discussion, copy/paste-ability is desirable. fixtures reduce copy/paste-ability, so should be used only in use cases where they provide non-trivial de-duplication or parametrization
  2. When i look at DataFrame() i know exactly what im dealing with. with a fixture, i have to go look that up.
  3. singleton fixtures make test signatures harder to grok. tz_naive_fixture is useful, empty_frame is not.

cc @jorisvandenbossche

@simonjayhawkins
Copy link
Member

closing as stale. @WillAyd feel free to reopen if you want to pursue this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd Awaiting requested review from WillAyd

Assignees
No one assigned
Labels
Testing pandas testing functions or related to the test suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use empty_frame fixture
6 participants
@jason3804 @pep8speaks @ShaharNaveh @jbrockmendel @WillAyd @simonjayhawkins