Skip to content

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

Closed
3 of 4 tasks
SaturnFromTitan opened this issue Feb 15, 2020 · 15 comments
Closed
3 of 4 tasks

pandas/conftest.py refactoring #31989

SaturnFromTitan opened this issue Feb 15, 2020 · 15 comments
Labels
Testing pandas testing functions or related to the test suite

Comments

@SaturnFromTitan
Copy link
Contributor

SaturnFromTitan commented Feb 15, 2020

As stated in this review comment from jreback, we should address some refactoring issues in pandas/conftest.py:

  • create the indices fixture using tm.all_index_generator as suggested by jbrockmendel in this comment
  • all fixtures and functions should have a docstring
  • add sections, i.e. "some text markers so reading the file is easy"
  • remove/avoid imports from conftest.py
@SaturnFromTitan
Copy link
Contributor Author

SaturnFromTitan commented Feb 15, 2020

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.

@jorisvandenbossche
Copy link
Member

remove/avoid imports from conftest.py

What do you mean with this?

@simonjayhawkins simonjayhawkins added the Testing pandas testing functions or related to the test suite label Feb 15, 2020
@simonjayhawkins
Copy link
Member

remove/avoid imports from conftest.py

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)

@jorisvandenbossche
Copy link
Member

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.

@jreback
Copy link
Contributor

jreback commented Feb 15, 2020

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
everything is a fixture

@jorisvandenbossche
Copy link
Member

everything is a fixture

You maybe want that, but it's not the reality ;)

@jreback
Copy link
Contributor

jreback commented Feb 15, 2020

everything is a fixture

You maybe want that, but it's not the reality ;)

exactly so why exacerbate it - we need a path to consistency - this is it

@jbrockmendel
Copy link
Member

BTW, importing from conftest.py is much easier to understand/find and more explicit than the magic of pytest.

+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

@jorisvandenbossche
Copy link
Member

Moving things out of conftest we want to import is fine for me as well (although pandas._testing is already a huge file, so maybe we should make it a module)

@MarcoGorelli
Copy link
Member

we could consider adding a code check rule to enforce this

xref #30914 :)

@SaturnFromTitan
Copy link
Contributor Author

Looking into this right now and pondering on

add sections, i.e. "some text markers so reading the file is easy"

Adding such text markers is always an indicator for too large files. Instead, I suggest to create a directory _conftest with files:

  • conftest_dtypes.py
  • conftest_autouse.py
  • ...
  • __init__.py (* importing everything from the other files)

and then use a simple from pandas._conftest import * in pandas/conftest.py.

What do you think @jreback @simonjayhawkins @jorisvandenbossche?

@jorisvandenbossche
Copy link
Member

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)

@simonjayhawkins
Copy link
Member

agree with @jorisvandenbossche for now. adding sections to conftest.py to address #31701 (comment) may help make the case for splitting in the future

@jreback
Copy link
Contributor

jreback commented Mar 17, 2020

+1 in adding commented sections but agree split not needed yet

@SaturnFromTitan
Copy link
Contributor Author

Closing this. I'll handle the remaining task "remove/avoid imports from conftest.py" in #30914

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing pandas testing functions or related to the test suite
Projects
None yet
Development

No branches or pull requests

6 participants