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

Add altair sphinx extension back to package? #3051

Closed
jtilly opened this issue May 11, 2023 · 15 comments
Closed

Add altair sphinx extension back to package? #3051

jtilly opened this issue May 11, 2023 · 15 comments

Comments

@jtilly
Copy link
Contributor

jtilly commented May 11, 2023

With version 5.0.0 (thank you for the release! 🥳) the altair sphinx extension was removed from the package. It used to be importable from altair.sphinxext. Would it be possible to bring that back?

There is a small number of projects on GitHub that use the extension in their own code base: https://github.com/search?q=%22altair.sphinxext%22&type=code including one of ours.

Thank you!

@jedbrown
Copy link
Contributor

We also use this, love altair, and would rather not copy sphinxext into our project. If this can't be re-added, what is the migration plan for projects that have been using altair.sphinxext?

@binste
Copy link
Contributor

binste commented May 11, 2023

Move of sphinxext to top-level happened in #2792. Honestly, I was simply not aware when reviewing it that other packages use this as well, although I can see now that it makes sense...

I think the main goal of that PR was to get the tests out of the altair module into a separate folder and altairgallery needs to import some functionality from the tests to fetch the example charts and so it made sense to lift it out as well. Also, sphinxext relies on packages, such as sphinx and docutils, which are not part of the Altair requirements.

How about we only move the altairplot directive back into altair.sphinxext as it does not depend on the tests folder? We can add ImportErrors with some context if docutils or sphinx are not installed. altairgallery only shows up in Altair forks so it could be left in a separate folder. I could work on this.

@jedbrown
Copy link
Contributor

Thanks, that would be great. And if you really don't want it included with the rest of altair, it could be a separate package altair-sphinxext on PyPI.

@jtilly
Copy link
Contributor Author

jtilly commented May 11, 2023

How about we only move the altairplot directive back into altair.sphinxext as it does not depend on the tests folder?

Yes, that would be great!

@binste
Copy link
Contributor

binste commented May 11, 2023

I'll wait for @mattijn's input before working on this.

@mattijn
Copy link
Contributor

mattijn commented May 11, 2023

I was unaware that there were other packages depending on the sphinxext module when moving it out of Altair package.

I've seen repositories struggling how to make these Altair plots appear in documentation. If it is a separate package I think it will make it easier to discover, install and use in other projects.

For the short-term, we can move it back as @binste described including and add a deprecation warning and setup a separate sphinxext-altair repository.

@binste
Copy link
Contributor

binste commented May 12, 2023

Using GH Search I can find only 5 actively maintained repositories which rely on altairplot:

I could move the altairplot functionality to sphinxext-altair in a few days (maybe this weekend) and so I'm wondering if it's even worth it to move back altairplot into altair with a deprecation warning. Might be less work for everyone if we directly go for the separate package. glum and libCEED maintainers are part of the discussion here and for the other 3 repos I can create issues and show how to switch to sphinxext-altair once it's out. Ok for you @mattijn or do you see an issue with this approach?

@joelostblom
Copy link
Contributor

I'm in favor of moving it directly into a separate package like you suggested.

@jedbrown
Copy link
Contributor

Agreed. We also use it in projects on GitLab, but people who have been using altair.sphinxext in CI will have pinned their versions already and can find the new package when they try to upgrade. A note in the Altair release notes would be helpful as that's the first place I looked to see why our docs CI was broken (before coming here).

@mattijn
Copy link
Contributor

mattijn commented May 12, 2023

@binste, I created https://github.com/altair-viz/sphinxext-altair. I think that should be sufficient to make a start with the transition?

@binste
Copy link
Contributor

binste commented May 13, 2023

Published https://github.com/altair-viz/sphinxext-altair on pypi. You can install it with pip install sphinxext-altair and use it with:

extensions = [
    ...
    "sphinxext_altair.altairplot",
    ...
]

This should give you the same functionality as before. Let me know how it goes! I also added a note to the release on github and will do the same for the docs.

@jtilly
Copy link
Contributor Author

jtilly commented May 13, 2023

Very cool, thank you! Is it okay if I turn this into a conda package (i.e. I would open a PR at https://github.com/conda-forge/staged-recipes)?

@binste
Copy link
Contributor

binste commented May 13, 2023

I've never created a conda package so this would be great, thank you!

@jtilly
Copy link
Contributor Author

jtilly commented May 13, 2023

PR is here: conda-forge/staged-recipes#22811

Update: available on conda-forge now, see https://anaconda.org/conda-forge/sphinxext-altair

@jtilly
Copy link
Contributor Author

jtilly commented May 15, 2023

Thank you @binste and @mattijn for creating https://github.com/altair-viz/sphinxext-altair/ so quickly! This resolves the issue for me 🥳

@jtilly jtilly closed this as completed May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants