-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
Document our pytest fixtures #19159
Comments
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 < ***@***.***> wrote:
It's quite a bit of missing docs. Two questions arise:
1. Where to start?
2. 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
<#19159 (comment)>,
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 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 < ***@***.***> 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
<#19159 (comment)>,
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 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
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. |
|
Partially addresses: pandas-dev#19159
Is there anything I can do to contribute? |
I'm going to take a chunk out of some of them... |
Hello, is this issue still open? I am a beginner contributor and want to help! |
There are still a lot of fixtures that need to be fixed. I am a beginner contributor and would like to help with this. I am starting with the fixtures in |
It'd be nice if all our fixtures had docstrings stating
From
pytest --fixtures
:The text was updated successfully, but these errors were encountered: