-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Fix doctests #4408
Fix doctests #4408
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome, thanks a lot @keewis !
Is there a tool to do this or you had to copy and paste all the results?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a tool to do this or you had to copy and paste all the results?
that was a entirely manual process. However, it should be possible to write a script using doctest.DoctestParser
, doctest.DoctestFinder
and doctest.DoctestRunner
(and I would be surprised if there's nothing like that already).
Well, thanks for doing all that! On the tool, I agree that would be great. Here's something I did in a day that aimed to do it for any value rather than doctests. Would be easier and less complicated for doctests: https://github.com/max-sixty/pytest-accept |
shall we merge and deal with the effects of #4409 later? |
I tried to fix the remaining doctests that were unrelated to #4409, so those need a review first (especially the |
wow. The RTD check fails due to:
|
Looks great! I raised an eyebrow about the Re the warning, we could add an |
yeah, well, I didn't want to leave files lying around. Not sure if there's a better way to do that. Maybe if there's a way to have
I doubt that would fix it (maybe I'm missing something), these are triggered by |
the main build is also failing so I guess this is unrelated. There is a |
which makes manually removing paths unnecessary
@max-sixty, I removed the call to |
@@ -36,3 +36,6 @@ def add_standard_imports(doctest_namespace): | |||
|
|||
# always seed numpy.random to make the examples deterministic | |||
np.random.seed(0) | |||
|
|||
# always switch to the temporary directory, so files get written there | |||
tmpdir.chdir() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, nice idea
Great! To the extent the RTD build failure is not from this branch, shall we merge this now? |
(i.e. feel free to merge!) |
the failing hypothesis test looks unrelated, so I'm merging. Edit: it seems RTD was fixed |
Thanks a lot @keewis ! This is a big step forward! |
should we add a CI that checks that we don't regress? |
Sounds good to me. Thanks for working on this. |
okay, then let's wait for #4409 which should fix the remaining three doctest failures. |
For sure re CI! |
This is an attempt to fix the examples in our docstrings. With this, the output of:
is
remaining warnings and errors
where
DataWithCoords.pipe
andDataset.filter_by_attrs
fail mostly because the order of the coords is not deterministic. Should we sort before formatting the coords?Does anyone know what
ds
insave_mfdataset
should look like? Also, how do we fix the construction ofOuterIndexer
andVectorizedIndexer
?isort . && black . && mypy . && flake8
whats-new.rst
Edit: if we merge #4409 before this, we can make sure all doctests pass.