wpilibsuite / sphinxext-opengraph

Sphinx extension to generate unique OpenGraph metadata
https://sphinxext-opengraph.readthedocs.io
Other
69 stars 25 forks source link

ENH: Add social card previews #88

Closed choldgraf closed 1 year ago

choldgraf commented 1 year ago

This adds social media card previews to this extension. It's still a work in progress, but opening now for feedback at implementation. If people are +1 then I can improve the docs + tests.

Here's an example of what it looks like.

chrome_3vZzptEKdo

Here's a demo page:

https://sphinxext-opengraph--88.org.readthedocs.build/en/88/socialcards.html

Implementation

It uses Matplotlib to generate a PNG "social media card" for each page of the documentation and links it via og:image (unless an OG image is given by the user manually).

It has some basic customization but is meant to work out-of-the-box, and is heavily inspired by GitHub's social card preview design.

To get this working, I also made a few improvements to the development workflow for the theme, which I'll summarize below:

To do

Refs:

choldgraf commented 1 year ago

I believe I've addressed the comments above - here's what I've done:

(I squashed a few commits together to make review easier, thus the force-push)

choldgraf commented 1 year ago

I've updated the tests for this, so they should now be passing. The behavior of this PR should only trigger if the user hasn't specified any of the other image options.

I've also added a tests job to the noxfile to easily run tests.

I think this is pretty much ready to go now, as long as a round of reviews is +1 on it.

cc @TheTripleV can you please start the test suite? It isn't auto-running for me.

choldgraf commented 1 year ago

Anybody know why the matplotlib install is failing? Here's the error message:

ERROR: Could not find a version that satisfies the requirement matplotlib (from sphinxext-opengraph) (from versions: none)

Also, can I just get a note of whether folks are enthusiastic about merging this PR eventually or not? I am perfectly happy to just release this functionality as a standalone Sphinx extension, that was my original intention until I was asked to push this upstream instead. But if folks are busy and don't have time to review or maintain this, that's fine. Just let me know - I know that we are all busy so it's OK if it takes some time, but I don't want to sink a bunch of hours into this if folks aren't enthusiastic about getting the functionality in.

hugovk commented 1 year ago

Anybody know why the matplotlib install is failing? Here's the error message:

ERROR: Could not find a version that satisfies the requirement matplotlib (from sphinxext-opengraph) (from versions: none)

There's an earlier job called build-wheel where it builds the sphinxext-opengraph wheel and uploads it as an artifact. Matplotlib is installed here:

      - name: Install dependencies
        run: |
          set -xe
          python -VV
          python -m site
          python -m pip install --upgrade pip setuptools wheel
          python -m pip install -r dev-requirements.txt
      - name: Install package
        run: |
          python -m pip install .
      - name: Build wheel
        run: |
          python -m pip install build
          python -m build
      - name: Upload sdist and wheel artifacts
        uses: actions/upload-artifact@v3
        with:
          name: my-dist
          path: dist/*

Then, each of the fresh parallel test jobs (for each Python, Sphinx and OS) download the wheel artifact, and installs it:

      - name: Install dependencies
        run: |
          python -VV
          python -m site
          python -m pip install --upgrade pip setuptools wheel
          python -m pip install -r dev-requirements.txt
          python -m pip install "sphinx${{ matrix.sphinx-version }}"
      - name: Download sdist and wheel artifacts
        uses: actions/download-artifact@v3
        with:
          name: my-dist
          path: dist
      - name: Install downloaded wheel
        run: |
          python -m pip install --only-binary=:all: --no-index --find-links=dist sphinxext-opengraph

But it's done in a way where it has no access to PyPI, and only installs things already available.

So I think you may need to include matplotlib in the "Install dependencies" step of the test job? Near where we also install Sphinx.


The build-wheel step is done for a single Python/OS version, then re-used for all test jobs across each Python/OS version. I wonder if installing a matplotlib wheel for a single Python/OS will cause problems with the later matrix? One way is to test and find out.


I'm also wondering whether always installing matplotlib will cause problems for some, especially if they don't intend to use these generated previews. They'll be able to disable at runtime via conf.py config, but they cannot disable the matplotlib install via setup.py. Possibly for people building docs for a Python/OS without a binary wheel for matplotlib, it may fail to build from sdist?

TheTripleV commented 1 year ago

Sorry for the delays. I started a code review earlier but never finished. I think it looks good. I was going to suggest a few lines of syntax changes but it's mostly lgtm for me.

If it's ok with you, I can push those small changes into this branch in about a week or so.

TheTripleV commented 1 year ago

Replacing the command in "Install downloaded wheel" with someone more like python -m pip install dist/*.whl would work, right?

hugovk commented 1 year ago

Should this also add og:image:width and :height?

For example:

<meta property="og:image:width" content="1200">
<meta property="og:image:height" content="628">

Re: https://ogp.me/#structured

choldgraf commented 1 year ago

If it's ok with you, I can push those small changes into this branch in about a week or so.

Sure thing, no worries. Feel free to push to this branch. And I understand the delay, I just wanted to make sure folks are interested (and must admit I'd like to use this feature in a number of my sites)

Re: installing matplotlib, I can see an argument both ways. On the one hand it is an extra install, on the other this functionality would be best if it happened purely under the hood so that people didn't need to manually turn it on or change their config.

Given that matplotlib is such a common library in the python world, maybe we can give a shot at adding the dependency, and then if there are complaints it could be moved to an optional dependency in a future pr? You could also try the same functionality with python image library, but I don't know how to do that which is why I used matplotlib

hugovk commented 1 year ago

Re: installing matplotlib, I can see an argument both ways. On the one hand it is an extra install, on the other this functionality would be best if it happened purely under the hood so that people didn't need to manually turn it on or change their config.

We're using this extension for CPython docs:

These images are great, it it would be nice to include them in the CPython docs (at the moment we're just using a single Python logo as the OG image), although there are lots of pages so it generates 482 images, taking up 41 MB (as a separate point, perhaps we can adjust the PNG options to squeeze these a bit). I need to check with others if our deploy is fine with this extra weight.

Anyway, we can disable the image build, but one place where Matplotlib being installed by default could be problematic is on our CI. We build Python from main (currently 3.12) to run doctests:

https://github.com/python/cpython/blob/2161bbf243983c625e8f24cdf43b757f2a21463b/.github/workflows/doc.yml#L56-L80

Given that matplotlib is such a common library in the python world, maybe we can give a shot at adding the dependency, and then if there are complaints it could be moved to an optional dependency in a future pr?

Yes, and Matplotlib has plenty of wheels, but of course doesn't have any yet for Python 3.12, so it might fail building from source (or be very slow). Plus it has dependencies at least for NumPy and Pillow, which are in the same position.

You could also try the same functionality with python image library, but I don't know how to do that which is why I used matplotlib

I actually had a go https://github.com/hugovk/pixel-tools/blob/main/og_image.py at that recently before we decided the simple Python logo was better. Your version is much better than what this produces!

Anyway, Pillow similarly has lots of wheels but not (yet) 3.12 so is in a similar boat to Matplotlib and NumPy.

Having said all this, for our doctest CI, perhaps we could skip the sphinxext-opengraph install entirely and sidestep the whole issue. We already have a conditional on adding the extension in conf.py (link above) in case downstream redistributors haven't installed it.


(We're also using this extension for the CPython developer's guide, with 52 images at 4.3 MB. Demo build here: https://hugovk-devguide.readthedocs.io/en/ogp_social_cards/)

choldgraf commented 1 year ago

Hey all - just FYI I am going on paternity leave in about 3 or 4 weeks. I'd like to get as many open PRs as possible wrapped up before then. If it doesn't seem like folks have time to look at this before then, I am going to focus my efforts on releasing a separate sphinx extension that uses sphinxext-opengraph under the hood.

Daltz333 commented 1 year ago

@TheTripleV

Daltz333 commented 1 year ago

@choldgraf IMO, I'd prefer to have it enabled by default and have an extra dependency requirement. I don't think it's uncommon in the Python world to have cross-dependencies, and the matplotlib API is usually quite stable, so unless a user reports upstream (users should always pin dependencies and sub-dependencies anyways), in-which case we can resolve this on a case-by-case basis.

We should resolve the CI issue by adding the matplotlib to the test install steps. Assuming tests pass and a basic functionality check, I'm fine for merge unless @TheTripleV is able to find time to make wanted changes. If that takes too long, it can be a separate PR.

I'd prefer not exposing a separate extension to PyPi. My opinion is that the extension should be usable (for us) without configuration. This allows projects like Juypter Books to install this and gain behavior for all users without configuration (assuming a RTD build environment).

choldgraf commented 1 year ago

I've added matplotlib to dev-requirements.txt but I believe somebody needs to approve and run the test suite to see if this fixes things

Daltz333 commented 1 year ago

@choldgraf can you rebase on upstream, update black, and then run black so that check passes? There also seems to be another test failure

Daltz333 commented 1 year ago

Once all tests pass, I'll bump to 0.8.0. I'm reserving 1.0.0 for any breaking changes we want to do.

choldgraf commented 1 year ago

OK, I have:

Daltz333 commented 1 year ago

I plan to merge this today if all goes right.

Daltz333 commented 1 year ago

I'll fix black in a separate PR.

choldgraf commented 1 year ago

Argh the windows builds are off, one second I will fix that.

choldgraf commented 1 year ago

@Daltz333 can you try re-running the tests?

Daltz333 commented 1 year ago

Reran.

choldgraf commented 1 year ago

Wanna try one more? I just blackified everything because I realized I'd only run it on the module and not the other python files.

Daltz333 commented 1 year ago

Reran

choldgraf commented 1 year ago

OK I think I figured out why it wasn't fixed before, wanna try one last time?

hugovk commented 1 year ago

@choldgraf Tip: you can also enable GitHub Actions on your own fork at https://github.com/choldgraf/sphinxext-opengraph/actions/ and won't need to wait for someone to press the magic button for them to run there :)

choldgraf commented 1 year ago

Oh wow I had no idea that was possible, OK I enabled it over here and will ping you if it's passing so you don't have to keep pressing the button

https://github.com/choldgraf/sphinxext-opengraph/actions/runs/4106496890

choldgraf commented 1 year ago

Ok build is happy - try running again!

Daltz333 commented 1 year ago

Pending testing at https://github.com/wpilibsuite/frc-docs/pull/2189

Daltz333 commented 1 year ago

Hm, it looks the extension is not injecting the metadata in both the documentation here and the linked PR above. @choldgraf

image

choldgraf commented 1 year ago

It does look like the metadata is there (look at the value for og:image), but the image doesn't exist because it's using the ogp_site_url as the base for the image link, and that image does not exist at the link yet because this PR isn't merged.

e.g.: https://www.opengraph.xyz/url/https%3A%2F%2Fsphinxext-opengraph--88.org.readthedocs.build%2Fen%2F88%2F

So for example, for the social cards page here's the value of og:image from that page:

https://sphinxext-opengraph.readthedocs.io/en/latest/_images/social_previews/summary_socialcards_e50aee1b.png

And if we replace the prefix with the PR preview link to the following:

https://sphinxext-opengraph--88.org.readthedocs.build/en/88/_images/social_previews/summary_socialcards_e50aee1b.png

Here it is:

choldgraf commented 1 year ago

wooo!