Skip to content

CLN: collect ordered fixtures in pandas.conftest #26082

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
Merged
merged 2 commits into from
Apr 16, 2019
Merged

CLN: collect ordered fixtures in pandas.conftest #26082

merged 2 commits into from
Apr 16, 2019

Conversation

topper-123
Copy link
Contributor

@topper-123 topper-123 commented Apr 14, 2019 •
edited
Loading

Currently, Categorical .ordered, CategoricaDtype.ordered and similar is tested using different fixtures + parametrization in some places. This PR collects the ordered fixtures/parametrizations and places a common ordered fixture in pandas.conftest.

@topper-123 topper-123 force-pushed the clean_ordered branch 3 times, most recently from 0cf776e to e37722b Compare April 14, 2019 11:34
@topper-123 topper-123 changed the title CLN: use fixture for ordered CLN: collect ordered fixtures in pandas.conftest Apr 14, 2019
WillAyd
WillAyd reviewed Apr 14, 2019
View reviewed changes
pandas/conftest.py Outdated
@@ -126,6 +126,12 @@ def observed(request):
return request.param


@pytest.fixture(params=[True, False, None])
def ordered(request):
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 this could be more generic than ordered since it would have applications for other parameters. Maybe something like optional_bool would be more fitting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, but there are also benefits of naming the fixture after the parameter being parametrized and not sharing. For example, if e.g. if 10 parametrizations would share a fixture and then one would change its signature, it would be more difficult to find where its fixture is used.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @topper-123 here. This is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is consistent as it’s naming is specific to its use; could name it ordered_fixture though as we use this particular nomenclature

@topper-123 topper-123 marked this pull request as ready for review April 14, 2019 22:05
@gfyoung gfyoung added Clean Testing pandas testing functions or related to the test suite labels Apr 14, 2019
jreback
jreback requested changes Apr 14, 2019
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

see comment

@codecov
Copy link

codecov bot commented Apr 16, 2019

Codecov Report

Merging #26082 into master will decrease coverage by 51.22%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #26082       +/-   ##
===========================================
- Coverage   91.96%   40.73%   -51.23%     
===========================================
  Files         175      175               
  Lines       52405    52405               
===========================================
- Hits        48192    21345    -26847     
- Misses       4213    31060    +26847
Flag Coverage Δ
#multiple ?
#single 40.73% <ø> (-0.14%) ⬇️
Impacted Files Coverage Δ
pandas/io/formats/latex.py 0% <0%> (-100%) ⬇️
pandas/io/sas/sas_constants.py 0% <0%> (-100%) ⬇️
pandas/core/groupby/categorical.py 0% <0%> (-100%) ⬇️
pandas/tseries/plotting.py 0% <0%> (-100%) ⬇️
pandas/tseries/converter.py 0% <0%> (-100%) ⬇️
pandas/io/formats/html.py 0% <0%> (-99.37%) ⬇️
pandas/io/sas/sas7bdat.py 0% <0%> (-91.16%) ⬇️
pandas/io/sas/sas_xport.py 0% <0%> (-90.1%) ⬇️
pandas/core/tools/numeric.py 10.44% <0%> (-89.56%) ⬇️
pandas/io/s3.py 0% <0%> (-89.48%) ⬇️
... and 130 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aad7739...781773b. Read the comment docs.

@codecov
Copy link

codecov bot commented Apr 16, 2019 •
edited
Loading

Codecov Report

Merging #26082 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26082      +/-   ##
==========================================
- Coverage   91.96%   91.95%   -0.01%     
==========================================
  Files         175      175              
  Lines       52405    52405              
==========================================
- Hits        48192    48188       -4     
- Misses       4213     4217       +4
Flag Coverage Δ
#multiple 90.51% <ø> (ø) ⬆️
#single 40.73% <ø> (-0.14%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 75% <0%> (-12.5%) ⬇️
pandas/core/frame.py 96.9% <0%> (-0.12%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aad7739...781773b. Read the comment docs.

@topper-123
Copy link
Contributor Author

I’ve changed the name of the fixture.

@jreback jreback added this to the 0.25.0 milestone Apr 16, 2019
@jreback jreback merged commit a7a4c7d into pandas-dev:master Apr 16, 2019
@jreback
Copy link
Contributor

jreback commented Apr 16, 2019

thanks @topper-123

yhaque1213 pushed a commit to yhaque1213/pandas that referenced this pull request Apr 22, 2019
@topper-123 @yhaque1213
CLN: collect ordered fixtures in pandas.conftest (pandas-dev#26082)
3b68936
@topper-123 topper-123 deleted the clean_ordered branch May 8, 2019 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd left review comments

@jreback jreback jreback approved these changes

@gfyoung gfyoung gfyoung approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@topper-123 @jreback @WillAyd @gfyoung