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

🐛 FIX: fix undefined sphinx var "meta" in jinja #395

Merged
merged 2 commits into from
Apr 26, 2021

Conversation

drammock
Copy link
Collaborator

closes #394

As far as I can tell, the site still renders fine after this change, but I would welcome a second pair of eyes.

@choldgraf
Copy link
Collaborator

Hmmm - I think the question is what happened to meta - did it get deprecated in jinja or something?

@drammock
Copy link
Collaborator Author

I didn't see anything about it in the changelog...

@larsoner
Copy link
Contributor

We are on a prerelease RC1 (and before that alpha):

https://pypi.org/project/Jinja2/#history

Might be worth looking through their issue tracker and opening an issue that either this is a BUG or a DOC problem at their end

@jorisvandenbossche
Copy link
Member

@drammock is there a full log of the doc build where you are seeing the error? (#394 only has the actual error)

It might be that meta is not defined for some special pages like genindex (that's the page where it is failing with this error in sphinx-themes/sphinx-themes.org#70)

@choldgraf
Copy link
Collaborator

Good point @jorisvandenbossche - I bet that you're right. As along as we are confident that we won't miss config elsewhere, then I'm +1 on this. I suppose that if we didn't pass crucial config in other places as a result of removing this, then it'd show up as errors elsewhere in the test suite

@drammock
Copy link
Collaborator Author

@jorisvandenbossche
Copy link
Member

Thanks, so there it also errored for the genindex page.

I am not sure why it hits us now and not before, but in any case what this PR does (is undefined check) is also how eg rtd theme is doing it: https://github.com/readthedocs/sphinx_rtd_theme/blob/9df65f15c5f62ac5a847c505f559c00586316936/sphinx_rtd_theme/breadcrumbs.html#L1

@choldgraf
Copy link
Collaborator

then I say let's :shipit:

@bollwyvl
Copy link
Collaborator

Two things:

  • can we add a test?
  • should we add a canary excursion with pip install --pre to catch some of these things sooner?
    • or perhaps more targetd just our runtime dependencies?

@pradyunsg
Copy link
Contributor

FWIW, meta comes from Sphinx (see https://www.sphinx-doc.org/en/master/templating.html#meta).

As noted already, it can be undefined for some pages, which is why things are failing for sphinx-themes.org. :)

@choldgraf
Copy link
Collaborator

I guess that the test for this should entail building a sphinx site with a genindex (or some other page without a meta tag) and making sure it doesn't error. Does this not already happen in our test suite?

@drammock what do you think?

@jorisvandenbossche
Copy link
Member

I suppose the demo docs / tests already do that, but we need an environment with the dev versions of sphinx/jinja to catch this

@choldgraf
Copy link
Collaborator

Ah - so perhaps we need to add another matrix dimension here for Sphinx version?

https://github.com/pydata/pydata-sphinx-theme/blob/master/.github/workflows/tests.yml#L65

Reasonable to strive to keep up with the latest 2 major versions in general + the pre-release (so, 2.x, 3.x, 4.xa)?

@jorisvandenbossche
Copy link
Member

Ah - so perhaps we need to add another matrix dimension here for Sphinx version?

Yes, it might indeed be good to test against multiple versions of sphinx.
For now, short term, I quickly added a build that simply installs any pre-release version if available of sphinx and jinja2 (it's actually jinja2 that is causing the error here, not sphinx) -> #402

@jorisvandenbossche jorisvandenbossche changed the title fix undefined jinja var "meta" 🐛 FIX: fix undefined sphinx var "meta" in jinja Apr 26, 2021
@jorisvandenbossche
Copy link
Member

So CI was failing in #402, and passing now with the fix in this PR. So merging.

Thanks @drammock !

@jorisvandenbossche jorisvandenbossche merged commit 3455c23 into pydata:master Apr 26, 2021
@jorisvandenbossche
Copy link
Member

I also made a 0.6.2 release with this fix (https://github.com/pydata/pydata-sphinx-theme/releases/tag/v0.6.2)

@choldgraf
Copy link
Collaborator

Woooo! Thanks @drammock for fixing this!

@jorisvandenbossche
Copy link
Member

(BTW, Chris, there is still something very strange with your git ;) For the previous 0.6.1 release, the release commit (f54466a) was this time done 3 days in the past .. For the 0.6.0 release you "only" did it one day in the past ;))

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.

theme error with Jinja 3.0.0rc1
6 participants