Open TomAugspurger opened 6 years ago
It's quite a bit of missing docs. Two questions arise:
I think whatever is easiest for you. I would search for a fixture that's relatively simple to describe.
I think doing things one module at a time makes sense.
On Tue, Feb 20, 2018 at 7:54 AM, Ignacio Vergara Kausel < notifications@github.com> wrote:
It's quite a bit of missing docs. Two questions arise:
- Where to start?
- Where to stop or how to chunk it?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pandas-dev/pandas/issues/19159#issuecomment-366984064, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQHInYG9ViJRDo3zNppJ19ao9PizSGeks5tWs6kgaJpZM4RYYPM .
Another question, while diving through the fixtures I've found one fixture (maybe it's not the only one) that is repeated in the same test module. Particularly the tz
fixture in tseries/conftest.py
and in tseries/offsets/conftest.py
.
Is that by design or oversight?
Since the scope of this issue is just to add doctrings to the fixtures I would just repeat it in both fixtures.
Probably an oversight. You could remove the one from offsets/conftest.py and see if things break. I think that pytest's discover should find the one in the parent directory, though I may be wrong.
On Tue, Feb 20, 2018 at 9:13 AM, Ignacio Vergara Kausel < notifications@github.com> wrote:
Another question, while diving through the fixtures I've found one fixture (maybe it's not the only one) that is repeated in the same test module. Particularly the tz fixture in tseries/conftest.py and in tseries/offsets/conftest.py.
Is that by design or oversight?
Since the scope of this issue is just to add doctrings to the fixtures I would just repeat it in both fixtures.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/pandas-dev/pandas/issues/19159#issuecomment-367008617, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQHIm_G2Zeo-fyPu7pVRS7kQfj8Oo7yks5tWuEGgaJpZM4RYYPM .
This is not an oversight, more like not implemented. We want to move any duplicate fixtures to a higher level (e.g. to a top-level conftest). IF they are identical that is. Now this may cause issues in that there may be function names that cause this to break. So for now its ok to leave them, but maybe add a TODO: duplicate in the doc-string (so that we know to fix it).
I wrote some docstrings for the tseries set of tests, plus marking where I found the repeated tz
fixture throughout the test collection. I'd like to test out the contribution procedure with this small commit as it would be my first one.
Since this contribution doesn't close GH19159 I don't know exactly how to proceed.
@ivergara you can push a PR whenever you're ready. Just link back to this issue so we can track it.
Was there ever a PR opened for this? I'm interested in contributing.
Some work has been done, but there are still more.
You can run pytest --fixtures pandas
to see all the fixtures.
Ideally all of them would have docstrings, and we would raise an error if someone writes a new one w/o a docstring.
Thanks @TomAugspurger!
@MatthewChatham I did some... but haven't found time to do the whole process. Will try to test a PR this week.
Excellent. I'll try to take a look this week as well and at least make one commit ;)
@MatthewChatham as you can see my contribution got merged, but there is a lot more fixtures that need doctrings.
@ivergara nice work!
Can anyone point what fixture still needs to be fixed, I would like to fix some from the remaining ones.
$ pytest pandas/tests --fixtures
Is there anything I can do to contribute?
I'm going to take a chunk out of some of them...
It'd be nice if all our fixtures had docstrings stating
From
pytest --fixtures
: