CLN: update imports in tests #29275
CLN: update imports in tests #29275
Conversation
| per1 - Period("2011-02", freq="M") | ||
|
|
||
| @pytest.mark.parametrize("n", [1, 2, 3, 4]) | ||
| def test_sub_n_gt_1_ticks(self, tick_classes, n): | ||
| # GH 23878 | ||
| p1 = pd.Period("19910905", freq=tick_classes(n)) |
There was a problem hiding this comment.
I think doing this will conflict with what @SaturnFromTitan is or will be doing in other PRs. I think better to keep as pd.Period
There was a problem hiding this comment.
We didn't agree on a solution for this yet - so I don't mind. For now, I'm only focussing on imports from pandas.util.testing
There was a problem hiding this comment.
As long as Period is already in the namespace, I think this is a strict improvement. If later this gets reverted to only-pd.Period I'm mostly fine with that too.
One reason I like the not-pd pattern is that I can look at imports and get an idea of what is tested in this module.
There was a problem hiding this comment.
well i am +1 on not using the pd namespace except in a very few cases in tests (eg when we are actually testing the ls namespace)
we should simply use the
from pandas import Series...
pattern it is immediately obvious what is and what is not imported
and there is no confusion about what to use
There was a problem hiding this comment.
This is getting into personal development preferences but as a counter argument I sometimes find these imports very annoying. When debugging the pd namespace is almost always available, yet when copy pasting blocks of code that use this import machinery you then have to go up and see what is imported from top of the module.
But that aside... the bigger point is that (beyond tests, where this probably doesn’t matter) the from ... imports cause circular references and we will never be able to enforce those consistently. We end up in a weird state like #29133 where there isn’t a good rhyme or reason to where we place imports except for the fact that we tried it at one point and it caused a circular ref, so when then moved it somewhere else.
Ultimately I hope we don’t have to think about imports or circular references at all
There was a problem hiding this comment.
it’s not personal preference but just like we avoid * imports
having a global space is a bad idea
explicit is always better than implicit
There was a problem hiding this comment.
Ultimately I hope we don’t have to think about imports or circular references at all
+1. Fortunately that's not an issue for most of the tests.
| @@ -15,6 +15,7 @@ | |||
| import pytest | |||
|
|
|||
| import pandas as pd | |||
| from pandas import Timestamp | |||
There was a problem hiding this comment.
Should these just stay pd. since already imported or did you want to remove the preceding import?
There was a problem hiding this comment.
In a later step I'm looking to separate out the remaining pd.* usages into a separate not-just-tslibs test module
|
this is fine |
+1 on what @WillAyd said here. |
|
thanks @jbrockmendel |
Yep, but maybe not do more of this PRs before having such discussion then? |
Medium-term goal is to separate out tslibs-only parts of these tests to make sure we have sufficient coverage in isolation.