Skip to content
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

Use doctests for guide #2623

Merged

Conversation

dstansby
Copy link
Contributor

@dstansby dstansby commented Jan 2, 2025

This reverts using IPython for code examples in #2589, and is targeted to that branch (not main). See #2589 (review) for the motivation for not switching to IPython, but sticking with the doctest status-quo

@dstansby dstansby requested a review from jhamman January 2, 2025 16:22
@jhamman
Copy link
Member

jhamman commented Jan 2, 2025

Thanks for doing this @dstansby - how can we run the doc tests? Are they run automatically when you build the docs or in CI?

@dstansby
Copy link
Contributor Author

dstansby commented Jan 2, 2025

They can be turned on to run with pytest in the normal test runs. Do you think setting that up should block v3?

@dstansby
Copy link
Contributor Author

dstansby commented Jan 2, 2025

doctests are being run on v2 (see #2552 where I had to fix some recently), so there should be a pattern to copy there to enable them for v3.

@jhamman
Copy link
Member

jhamman commented Jan 2, 2025

okay, I think we should do that here before merging. The main reason I went with Ipython was to guarantee that the docs were reflecting exactly what Zarr produces. So we don't want to loose that.

@dstansby
Copy link
Contributor Author

dstansby commented Jan 2, 2025

I had a quick try, but it's not trivial (but probably not complicated either) to enable doctests. To keep things simple here I'd advocate enabling doctests as out of scope for this PR, and we can do it in a follow up to all the other PRs. FWIW I have run all the doctests locally to generate the output, so it should be correct at the moment.

@jhamman
Copy link
Member

jhamman commented Jan 2, 2025

@dstansby - can you describe the issues with getting the doc tests passing here? Since we're reverting work that ensures the doc can be run, I'd like to, at a minimum, understand why this route isn't working.

@dstansby
Copy link
Contributor Author

dstansby commented Jan 2, 2025

Since we're reverting work that ensures the doc can be run

I don't think this is stopping the doc build from working? The readthedocs failures in this PR are the same as in the branch it's based on (ie #2589)

I'm happy to either enable doctests in this PR, or in a follow up PR to #2589 as you prefer. I thought the preference was to merge #2589 ASAP, so I didn't want to hold up this (and therefore #2589) by working out the doctests, but I can do that (but maybe not today) if you think that should block #2589?

@jhamman
Copy link
Member

jhamman commented Jan 2, 2025

Let's enable doctests in this PR. Otherwise, we're going to immediately break things in the docs and not know it.

@dstansby dstansby mentioned this pull request Jan 2, 2025
@dstansby
Copy link
Contributor Author

dstansby commented Jan 2, 2025

👍 doctests enabled in this PR now

@jhamman jhamman merged commit dd17198 into zarr-developers:docs/3.0-user-guide Jan 2, 2025
6 checks passed
@dstansby dstansby deleted the turn-off-ipython branch January 2, 2025 23:03
dstansby added a commit that referenced this pull request Jan 3, 2025
* docs: split tutorial into multiple user guide sections

* Apply suggestions from code review

Co-authored-by: David Stansby <dstansby@gmail.com>

* respond to david's review

* add todos for remainig api changes

* docs: add new top level about page (#2592)

* docs: add new top level about page

* fixup

* fixup

* fixup

* docs: add docs on extending zarr 3 (#2597)

* docs: add docs on extending zarr 3

* Apply suggestions from code review

Co-authored-by: David Stansby <dstansby@gmail.com>

* move note up

* remove test.py (#2612)

* Note that whole directories can be deleted in LocalStore (#2606)

* fix: run-coverage command now tracks src directory (#2615)

* fix doc build

* Update docs/user-guide/extending.rst

---------

Co-authored-by: Norman Rzepka <code@normanrz.com>
Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: Davis Bennett <davis.v.bennett@gmail.com>

* Use doctests for guide (#2623)

* Use doctests for arrays.rst

* Use doctests for attributes.rst

* Use doctests for config.rst

* Use doctests for consolidated metadata

* Use doctests for groups.rst

* Use doctests for preformance.rst

* Use doctests for storage.rst

* Remove ipython config for docs

* Fix performance doctest output

* Enable doctests

* Add a doctest CI run

* Remove rmtrees

* Delete data dir before doctests

* Fix doctests

* fix doctests for arrays.rst

* fix doctests for consolidated_metadata.rst

* fixes for doctest

* tests

* debugging

* debugging

* debugging

* debugging

* debugging

---------

Co-authored-by: David Stansby <dstansby@gmail.com>
Co-authored-by: Norman Rzepka <code@normanrz.com>
Co-authored-by: Davis Bennett <davis.v.bennett@gmail.com>
Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants