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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependencies: add minimum docutils version #281

Merged
merged 1 commit into from
Feb 23, 2024
Merged

Dependencies: add minimum docutils version #281

merged 1 commit into from
Feb 23, 2024

Conversation

EFord36
Copy link
Contributor

@EFord36 EFord36 commented Feb 12, 2024

Closes #280

Versions lower than 0.20 cause rendering problems in the usage page with the bibliography, see #280.


馃摎 Documentation preview 馃摎: https://sphinx-hoverxref--281.org.readthedocs.build/en/281/

@EFord36
Copy link
Contributor Author

EFord36 commented Feb 12, 2024

the preview doesn't show this as fixed - but looking at the html from the preview, it's still using docutils 0.18.0:

<meta name="generator" content="Docutils 0.18.1: http://docutils.sourceforge.net/">

so the workflow must be caching the dependencies somehow.

@humitos
Copy link
Member

humitos commented Feb 12, 2024

The requirements are installed from docs/requirements.txt. See https://beta.readthedocs.org/projects/sphinx-hoverxref/builds/23418992/#219443307--14

@EFord36
Copy link
Contributor Author

EFord36 commented Feb 12, 2024

oops! Thanks for explaining, I'll run the pip-compile command and update the PR

@EFord36 EFord36 requested a review from a team as a code owner February 12, 2024 17:14
@EFord36
Copy link
Contributor Author

EFord36 commented Feb 12, 2024

Updated, and now the preview is fixed as expected 馃槃 Thanks for the help on the CI side, sorry for not noticing that requirements.txt file myself!

@humitos
Copy link
Member

humitos commented Feb 13, 2024

Thanks for the PR. No need to sorry 馃槈

Tests are broken because Sphinx<5 is broken due to their own dependencies (see sphinx-doc/sphinx#11890). Not sure what to do there -- we can probably pin all their dependencies to older versions, maybe?

pyproject.toml Outdated
Comment on lines 54 to 57
# not needed directly, but ends up
# in dependency graph and lower
# versions cause issues
"docutils>=0.20.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know what's the extension that doesn't support docutils>0.18?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's sphinxcontrib-bibtex:

mcmtroffaes/sphinxcontrib-bibtex@ba61382 (with follow up commit to fix error: mcmtroffaes/sphinxcontrib-bibtex@f60c7ce )

Although I must confess I haven't looked fully into what exactly causes the weird html output as I was sufficiently satisfied this was the problem.

Shall I edit the comment to something more specific, like:

Suggested change
# not needed directly, but ends up
# in dependency graph and lower
# versions cause issues
"docutils>=0.20.0",
# not needed directly, but
# lower versions cause issues with
# sphinxcontrib-bibtex, see
# https://github.com/mcmtroffaes/sphinxcontrib-bibtex/commit/ba613822a65cd685e02cc2692afc1dd0efa8b90f
"docutils>=0.20.0",

of course, an alternative is to just add a minimum version of sphinxcontrib-bibtex of 2.6.0 for the docs, where they have the relevant version pin for docutils themselves. However, that does exclude versions of sphinxcontrib-bibtex that work correctly (as long as they get the right version of docutils). What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinning to 2.6.0 looks like a clearer approach to me 馃憤馃徏 . You can explain this in a comment next to this pin.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! sorry for the delay, I'm on holiday this week, but should be able to keep on top of this now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enjoy your holidays! This can definitely wait. There is no rush 馃槈

@EFord36
Copy link
Contributor Author

EFord36 commented Feb 13, 2024

Thanks for the PR. No need to sorry 馃槈

Tests are broken because Sphinx<5 is broken due to their own dependencies (see sphinx-doc/sphinx#11890). Not sure what to do there -- we can probably pin all their dependencies to older versions, maybe?

Happy to add the pins for the relevant dependency - I just saw #279 though and your comment about doing a new release for Sphinx>=5.0 - I think maybe that's the better solution? Most end users will be using sphinx >5 anyway, so it would save force users for whom everything is working fine from using an older version of sphinx's dependencies than necessary for them.

what would you think of merging this pr 'as-is', or potentially modifying the tox setup to not test Sphinx versions below 5.0, and then following up with a release requirement Sphinx 5.0 or newer?

@humitos
Copy link
Member

humitos commented Feb 19, 2024

what would you think of merging this pr 'as-is', or potentially modifying the tox setup to not test Sphinx versions below 5.0, and then following up with a release requirement Sphinx 5.0 or newer?

I think we should do all of that, yes 馃槃

  • Merge this PR
  • Update tox to remove old tests
  • Release a new version that only supports Sphinx>=5

Would you like to work on these? I'm happy to continue reviewing and helping you here.

@EFord36
Copy link
Contributor Author

EFord36 commented Feb 19, 2024

what would you think of merging this pr 'as-is', or potentially modifying the tox setup to not test Sphinx versions below 5.0, and then following up with a release requirement Sphinx 5.0 or newer?

I think we should do all of that, yes 馃槃

  • Merge this PR
  • Update tox to remove old tests
  • Release a new version that only supports Sphinx>=5

Would you like to work on these? I'm happy to continue reviewing and helping you here.

Sure - I'll start work on a PR to update tox and update the pyproject.toml and changelog etc. for the new release.

For the actual release, shall I leave the running of the final flit publish to you?

For this PR, are you happy to merge now, or do you want to wait for the tox test update?

@humitos
Copy link
Member

humitos commented Feb 19, 2024

For the actual release, shall I leave the running of the final flit publish to you?

Yes.

For this PR, are you happy to merge now, or do you want to wait for the tox test update?

Let's wait for the tox updates so we are sure everything is on it's place 馃憤馃徏

@EFord36 EFord36 mentioned this pull request Feb 20, 2024
@EFord36
Copy link
Contributor Author

EFord36 commented Feb 20, 2024

@humitos I've updated this now, and all looks ok to me (assuming the tests pass), let me know if there's anything else you want me to do on it!

Otherwise the lack of a pin on docutils causes bad html rendering.

Docutils versions lower than 0.20 cause rendering problems in the usage page with
the bibliography, see #280.

upgrade docs/requirements.txt with pip-compile after the pyproject.toml
change.
@EFord36 EFord36 requested a review from humitos February 22, 2024 15:54
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! Thanks a lot 馃挴

@humitos humitos merged commit c853dd6 into readthedocs:main Feb 23, 2024
5 checks passed
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.

Broken rendering of 'usage' page on readthedocs
2 participants