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

Remove sphinx-autodoc-typehints, upgrade Myst & Sphinx #1153

Merged
merged 9 commits into from
Aug 16, 2024
Merged

Conversation

ewjoachim
Copy link
Member

Closes #1150

Successful PR Checklist:

  • Tests
    • (not applicable?)
  • Documentation
    • (not applicable?)

PR label(s):

@ewjoachim ewjoachim requested a review from a team as a code owner August 11, 2024 10:35
@github-actions github-actions bot added the PR type: documentation 📚 Contains documentation updates label Aug 11, 2024
Copy link

github-actions bot commented Aug 11, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  procrastinate
  app.py
  tasks.py
  testing.py
Project Total  

This report was generated by python-coverage-comment-action

@ewjoachim ewjoachim force-pushed the sphinx-update branch 3 times, most recently from 2fe440a to b1a69b6 Compare August 11, 2024 21:23
@ewjoachim
Copy link
Member Author

Sphinx 8 is only compatible with py3.10+, which is why it's complex

@medihack
Copy link
Member

Sphinx 8 is only compatible with py3.10+, which is why it's complex

And we want to retain Python >= 3.8 compatibility because Django 4.x supports the same version range?
Unfortunately, I don't understand the full story here. Why do you want to upgrade Sphinx and get rid of sphinx-autodoc-typehints? Is the functionality of the extension now fully built into Sphinx? (I can't find anything about it in the release notes).

@ewjoachim
Copy link
Member Author

ewjoachim commented Aug 13, 2024

We want to use Sphinx 8 for our own doc (it solves small issues that are interesting for us I think). We have no problem if users install procrastinate with older versions of Sphinx

In pyproject.toml, the extra dependency on sphinx is =* but the sphinx we have in our docs group is >=8 which ensures that the version in the lockfile is at least v8 but the lockfile is for the Procrastinate dev enc, Procrastinate users don't see it at all.

The only place where we actually need Sphinx >=8 is when we build the doc, which means we can only build the procrastinate doc with env with python 3.10+.

As far as I can tell, we can tell this to poetry with version markers and it will be able to track multiple versions of some package in the lockfile depending on the Python version we use. Just need to do that here. But yeah, complex :(

@ewjoachim
Copy link
Member Author

Unfortunately, I don't understand the full story here. Why do you want to upgrade Sphinx and get rid of sphinx-autodoc-typehints? Is the functionality of the extension now fully built into Sphinx? (I can't find anything about it in the release notes).

I want to upgrade Sphinx for multiple reasons:

  • if we do nothing, we'll only ever switch to v8 when we drop 3.9 which would be in more than a year, and by then Sphinx itself will probably have new majors. This means we'll never have recent Sphinxs.
  • yep, sphinx-autodoc-typehints is now covered by Sphinx itself (as far as I can tell) but I think it's been so for a few years
  • when I built on Sphinx 7, I noticed some places where the type annotations didn't display well. When I built with Sphinx 8, it seemed fixed (sorry, I don't remember the exact details)

It's not a problem if we choose not to upgrade Sphinx, it just felt that it makes sense to try and follow their latest releases.

If we manage to find a way to express this in pyproject.toml, great. Otherwise, there's no harm in just keeping the parts of the PR that fixes the docstrings & such

@ewjoachim
Copy link
Member Author

ewjoachim commented Aug 16, 2024

Ok, as long as Poetry has this bug, we won't be able to use Sphinx 8. I've reverted that part, the rest stands.

@ewjoachim ewjoachim merged commit cfe0aa6 into main Aug 16, 2024
11 checks passed
@ewjoachim ewjoachim deleted the sphinx-update branch August 16, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR type: documentation 📚 Contains documentation updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Be more consistent in the docstrings regarding the argument types
2 participants