-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN: Split test_window.py #27305
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
CLN: Split test_window.py #27305
Conversation
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.
Thanks - this module could definitely use a little refactor.
Not opposed to this move but thinking through if we should parametrize first instead
self.data = self._create_dtype_data(self.dtype) | ||
self.expects = self.get_expects() | ||
|
||
def test_dtypes(self): |
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.
Is this the only function being tested in this module? If so I think would make sense to try and parametrize this first before moving
|
||
def test_ragged_std(self): | ||
|
||
df = self.ragged |
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.
Good case for parametrization here (and a few functions below)
pandas/tests/window/test_window.py
Outdated
@@ -1,6 +1,6 @@ | |||
from collections import OrderedDict | |||
from datetime import datetime, timedelta | |||
from itertools import product | |||
from typing import Any |
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.
Not opposed to this but note that we haven't done annotations in any other test module nor do we check it (mypy.ini excludes these) so would be OK to leave out for now as well
pandas/tests/window/test_window.py
Outdated
|
||
|
||
@pytest.fixture(params=[None, 1]) | ||
def min_periods(request) -> Any: |
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.
Optional[int]
would be preferable as the return value here
@mroeschke @WillAyd let's do the straight split first, modifications later. ping on green. |
I think we merged something, can you merge master and ping on green. |
All green @jreback |
thanks @mroeschke |
it looks like this broke the ci. |
@simonjayhawkins can u put a link up |
Strange that my PRs based on earlier commit also fails. Does azure merge the PR into master before tesing? |
it looks like it. good to know. |
xref #19228, #26807
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
test_window.py
was getting fairly unwieldily. There a couple test classes that use a non-pytest-idomaticBase
class, so those test classes are kept together for now. I split out the rest of the test classes into separate files as they made sense.I think the classes in
test_window.py
need further untangling before aconftest.py
file can be made for the new directory.Open to additional quick win optimizations.
cc @jreback