executablebooks / sphinx-tabs

Tabbed views for Sphinx
https://sphinx-tabs.readthedocs.io
MIT License
263 stars 67 forks source link

RFE: please provide support for `jinja2` >= 3.0.0 #166

Closed kloczek closed 2 years ago

kloczek commented 2 years ago

https://github.com/executablebooks/sphinx-tabs/blob/b9fd7d81147c20da11960cb0f296ec9a55845c06/setup.py#L28

kloczek commented 2 years ago

Actually just found that this install time dependency cen be dropped

[tkloczko@devel-g2v sphinx-tabs-3.4.0]$ grep -r jinja2
setup.py:    install_requires=["sphinx>=2,<6", "pygments", "docutils~=0.17.0", "jinja2<3.1.0"],
foster999 commented 2 years ago

This was added in #164 because later versions of jinja2 seem to be incompatible with older versions of sphinx. Running the tests without that dependency specified shows that sphinx imports a depreciated jinja2 object.

We could make the requirement more specific if we can identify when the breaking change was included? Otherwise, this might be an issue for sphinx to support the latest versions of jinja2?

foster999 commented 2 years ago

So it looks like this has already been addressed in the latest versions of sphinx. I think our solution would be to follow sphinx and only officially support their latest version, because it would be too much faff to manage the dependencies for older sphinx versions

There's a comment on there suggesting that sphinx should pin the jinja2 version in older sphinx versions, but lack of this causes the issue here

kloczek commented 2 years ago
[tkloczko@devel-g2v g2v]$ pip show sphinx
Name: Sphinx
Version: 5.0.2
Summary: Python documentation generator
Home-page: https://www.sphinx-doc.org/
Author: Georg Brandl
Author-email: georg@python.org
License: BSD
Location: /usr/lib/python3.8/site-packages
Requires: alabaster, babel, docutils, imagesize, importlib-metadata, Jinja2, packaging, Pygments, requests, snowballstemmer, sphinxcontrib-applehelp, sphinxcontrib-devhelp, sphinxcontrib-htmlhelp, sphinxcontrib-jsmath, sphinxcontrib-qthelp, sphinxcontrib-serializinghtml
Required-by:

As you see sphinx has now jinnja2 on list of its dependencies.

foster999 commented 2 years ago

The issue here was due to jinja2 not being listed in their dependencies in older sphinx versions that we were trying to support. But I think this has highlighted that we don't need to try to support older versions, so thanks :)

kloczek commented 2 years ago

IIRC it is possible to make conditional dependencies. In case of sphins 5.x clearly that dependency is redundant.

foster999 commented 2 years ago

Jinja2 version no longer pinned from v3.4.1 👍🏻

kloczek commented 2 years ago

Thank you. Just added that PR to my build procedure and so far all look good 👍