-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
pandas/conftest.py refactoring #31989
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
Comments
If somebody wants to look into any of these, please feel free to do so - ideally with one PR per issue. If no one does so, I'll probably take it in the coming 2 weeks. |
What do you mean with this? |
we could consider adding a code check rule to enforce this. IIUC pytest already prohibits importing fixtures, but currently nothing stops us importing variables. in general, when doing this, it's probably an indicator that the fixtures are not used correctly (or in some cases maybe not designed appropriately) |
We also define variables (constants) in conftest.py that are used in tests, and those are not always fixtures and thus need to be imported. BTW, importing from conftest.py is much easier to understand/find and more explicit than the magic of pytest. |
we should never import from conftest for any reason it is non idiomatic ; and very arbitrary |
You maybe want that, but it's not the reality ;) |
exactly so why exacerbate it - we need a path to consistency - this is it |
+1 for not-magic. If there's something we need in conftest and want to be able to import, let's just put it in pandas._testing or something |
Moving things out of conftest we want to import is fine for me as well (although |
xref #30914 :) |
Looking into this right now and pondering on
Adding such text markers is always an indicator for too large files. Instead, I suggest to create a directory
and then use a simple What do you think @jreback @simonjayhawkins @jorisvandenbossche? |
I am slightly -1 on splitting conftest.py for now. It's already magical how fixtures get used and thus hard to know where they are defined. Splitting makes that even harder IMO (now, at some point it probably gets unavoidable, but right now I am not sure conftest.py is already annoyingly long) |
agree with @jorisvandenbossche for now. adding sections to conftest.py to address #31701 (comment) may help make the case for splitting in the future |
+1 in adding commented sections but agree split not needed yet |
Closing this. I'll handle the remaining task "remove/avoid imports from conftest.py" in #30914 |
As stated in this review comment from jreback, we should address some refactoring issues in
pandas/conftest.py
:tm.all_index_generator
as suggested by jbrockmendel in this commentconftest.py
The text was updated successfully, but these errors were encountered: