search

wpilibsuite / sphinxext-opengraph

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

Fix warning with ogp_image_alt default #29

Closed tcmetzger closed 3 years ago

tcmetzger commented 3 years ago

The way I had originally implemented support for ogp_image_alt led to a warning issued by sphinx make ("WARNING: The config value 'ogp_image_alt' has type 'str', defaults to 'bool'.") This pull request fixes the problem by making None the default value for this tag.

TheTripleV commented 3 years ago

I don't like that setting a variable to None enables functionality. Could we redo this logic so None means no alt, and "" means fallthrough?

tcmetzger commented 3 years ago

I'm happy to modify this in any way you want. However, there is one thing to consider:

In case an ogp_image is defined, but no ogp_image_alt is supplied (i.e. ogp_image_alt equals None) , the extension currently generates an og:image:alt tag based on site_name or title. The reason for this is that the official Open Graph Protocol says "If the page specifies an og:image it should specify og:image:alt." (https://ogp.me/#structured). Automatically generating an og:image:alt tag if an og:image is present is a direct implementation of the Open Graph Protocol.

If we change this behavior to only generate an og:image:alt tag if ogp_image_alt is set to a string or empty string, there will be cases where we end up with an og:image but no og:image:alt (and therefore deviate from what the Open Graph Protocol says).

Again, I'd be happy to adapt the code in any way you prefer, the decision is up to you as maintainers.

Daltz333 commented 3 years ago

I think that is reasonable, was there a different implementation that you wanted @ TheTripleV?