-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
PKG: Exclude data test files. #19535
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
Changes from all commits
4d77cd8
270e442
26e9b4b
1804bcc
7022152
080f000
151ffda
d9d6570
5849591
31fb0b6
9193f15
e897f11
9cf30fd
95cde7a
10ddddc
e1ea208
8616878
77bf77c
156e14b
f3f3662
2cd9706
aac3606
762a2d1
7c44b77
ad09951
6f02d6b
489f540
8b42d1c
ee4fefd
bac438c
84ccdbf
c4802db
7fd7660
c187f8b
632a61d
b5b70c7
9954bba
c771885
dd75270
dbe0c57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,11 @@ Documentation Changes | |
- | ||
- | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a ref here as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. General note: IMO it is not needed to always ask this of contributors, as this ref is only needed when we actually want to make an explicit link to it from within the rst files (and chances are quite high we will never do this). The ref can always be added at the moment one adds a link. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, but in general its a good practice |
||
Build Changes | ||
------------- | ||
|
||
- The source and binary distributions no longer include test data files, resulting in smaller download sizes. Tests relying on these data files will be skipped when using ``pandas.test()``. (:issue:`19320`) | ||
|
||
.. _whatsnew_0232.bug_fixes: | ||
|
||
Bug Fixes | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,8 +37,9 @@ | |
|
||
class TestPandasContainer(object): | ||
|
||
def setup_method(self, method): | ||
self.dirpath = tm.get_data_path() | ||
@pytest.fixture(scope="function", autouse=True) | ||
def setup(self, datapath): | ||
self.dirpath = datapath("io", "json", "data") | ||
|
||
self.ts = tm.makeTimeSeries() | ||
self.ts.name = 'ts' | ||
|
@@ -59,7 +60,8 @@ def setup_method(self, method): | |
self.mixed_frame = _mixed_frame.copy() | ||
self.categorical = _cat_frame.copy() | ||
|
||
def teardown_method(self, method): | ||
yield | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I learned that autouse fixtures can include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On the other hand, it also makes this more "complex" to understand IMO (the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two things
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've done this myself a few times so wanted to chime in. This approach is more in line with how pytest suggests doing setup/teardown (see here) so I think that's a +1 for it. It also gives you potential visibility into the context of the yield tests (ex: here I think you could replace the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, that's a good reason to use an autouse fixture here in this case. (you don't need to convince me of the benefit of fixtures in general :), however, I think many people are not that familiar with all those pytest special features and it has a steeper learning curve IMO, so there can be a balance in how fancy we go) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. When documenting this, I'll recommend against autouse for cases like this. I think it'd be better to just have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah we should really not use this pattern, rather changing to all fixtures. as a temporary workaround this ok, can you create an issue to 'fix' this properly though. |
||
|
||
del self.dirpath | ||
|
||
del self.ts | ||
|
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.
maybe should make a variable that holds all of these options for both the echo and the run to avoid duplication