-
Notifications
You must be signed in to change notification settings - Fork 20
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
docs: improve cross-referencing with intersphinx #203
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.
Thanks for doing this! Looks great overall. I had a few questions that I left as review comments but otherwise all 👍 from me.
Another idea: switch to MyST. It is a superset of restructurettext and also allows the source code for the documentation to be formatted (with Prettier). More importantly, with MySt, Jupyter notebooks can also make use of intersphinx.
I have been wanting to do this for awhile but never got to it. Is it easy to switch over and get working on RTD? Also what's the breakdown of MYST
vs jupyterbook
they can both be used to generate docs and book builds on myst? Finally, would this be in addition to this PR or in-place of?
__all__ = [ | ||
"scatter_selector", | ||
"scatter_selector_index", | ||
"scatter_selector_value", | ||
"RangeSlider", | ||
] | ||
|
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.
any reason to remove this?
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.
__all__
determines which functions are rendered in the API by sphinx-apidoc (and I guess autosummary too). If it's removed, all public members are included. The drawback: SliderBase
is now imported as well if you use from mpl_interactions.widgets import *
. That *
syntax should be avoided though.
Why did I remove it, and include SliderBase
? Sphinx-apidoc generates links to the base classes as well:
If those base classes are not available in the API, Sphinx will just raise some warnings. But I think those warnings should be avoided (hence 0efc4e0 and aafbcb0), so I included the SliderBase
class. As an alternative, we can list it under nitpicky_ignore
and put back the __all__
.
commands = | ||
make html | ||
|
||
[testenv:doclive] |
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.
Would using tox then be the best way to start rebuilding the docs? If yes then we should also update https://mpl-interactions.readthedocs.io/en/stable/Contributing.html
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.
Yeah sorry, it's only later that I notice the make watch
you defined
https://github.com/ianhi/mpl-interactions/blob/2eeda96afe6d2d9e6c7354818e7966f0f8a7ad43/docs/Makefile#L22-L23
I personally prefer tox though (it's basically a wrapper around make
), because you can run it from any directory in the repo. And in a way, you can use it as local CI.
Also I'm the creator/maintainer of https://github.com/matplotlib/matplotlib-extension-cookiecutter so I'll happily merge any similar changes there if you want to contribute them (or any other opinions you have about how that should work) |
Co-authored-by: Ian Hunt-Isaak <[email protected]>
for more information, see https://pre-commit.ci
Ah yes, you mentioned. Ok, I'll keep an eye on it. |
Oh and, good to hear! Nah can be done later on. That would indeed be a next 'upgrade'. For clarity:
I think myst-nb and sphinx-book-theme can be implemented separately from switching to myst files. |
Awesome - thanks for the explanation! Are you happy with where this is now? |
Yep, looks fine I think. |
Yup. That sounds fine, plus I think the way described there will still work. Thanks again! |
Closes #202
The commit 911ea15 is opinionated: it switches from
autosummary
tosphinx-apidoc
.The commit can be reverted/kicked out if the disadvantages outweigh the advantages.