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

Support Sphinx post-releases #1344

Closed
wants to merge 1 commit into from
Closed

Support Sphinx post-releases #1344

wants to merge 1 commit into from

Conversation

CreatingNull
Copy link

Fixes #1343, the way this string is being separated was not safe for Sphinx's pypi pre-release/post-release versions.

This includes current release 5.2.0.post0, but also applies to previous pypi beta releases such as 5.0.0b1

As the _ver_bugfix is not being used within templates, we can slice to only include major and minor components.
This avoids issues from the patch component being non-integer as well the len(sphinx_version.split('.')) being > 3.

Slices to ignore version info after minor release as this can be variable length or non-int.
@rkdarst
Copy link
Contributor

rkdarst commented Sep 25, 2022

From https://github.com/sphinx-doc/sphinx/issues
/10860#issuecomment-1257236167

RTD's theme should really use the version tuple attribute we provide, rather than somewhat clumsily introspecting the string version.

I guess that is sphinx.version_info, which simplifies the whole thing.

@rkdarst
Copy link
Contributor

rkdarst commented Sep 25, 2022

from sphinx/builders/html/__init__.py, in self.globalcontext is

            'sphinx_version': __display_version__,
            'sphinx_version_tuple': sphinx_version,

so I guess it is sphinx_version_tuple.

@rkdarst
Copy link
Contributor

rkdarst commented Sep 25, 2022

But that was only added in sphinx-doc/sphinx@426fdca, which was only added in 4.2 (2021-09-12), so we should definitely still parse the string for a few more years - at least, I would do more.

@AA-Turner
Copy link
Contributor

AA-Turner commented Sep 25, 2022

From sphinx-doc/sphinx/issues /10860#issuecomment-1257236167

RTD's theme should really use the version tuple attribute we provide, rather than somewhat clumsily introspecting the string version.

I guess that is sphinx.version_info, which simplifies the whole thing.

Yes, sphinx.version_info was added in Sphinx 1.2 -- this is what I'd suggest passing into your Jinja templates.

A

@AA-Turner
Copy link
Contributor

If you want to keep the logic in pure Jinja, you could gate the split/map operation on Sphinx versions prior to 4.2, and use the tuple from 4.2 onwards, I believe.

A

@CreatingNull
Copy link
Author

I raised this PR as an option to resolve the short-term issue, this is no longer required as Sphinx released 5.2.1.
Closing this PR in favor of someone more familiar with this project / Sphinx extensions to resolve properly.

Personal opinion is we shouldn't be checking the version to determine how to check the version.

Agree with @AA-Turner on his first comment, and since sphinx.version_info is being used for version checks in the python source, it'd make sense to also complete checks within Jinja templates using the data originated from that same tuple.

@pelson
Copy link
Contributor

pelson commented Sep 27, 2022

FWIW I implemented a fix in #1345.

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.

ValueError: too many values to unpack (expected 3)
5 participants