-
Notifications
You must be signed in to change notification settings - Fork 36
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
Restrict to Sphinx 5 #282
Restrict to Sphinx 5 #282
Conversation
Older Sphinx version have issues with Sphinx's dependencies itself, which isn't being fixed by the Sphinx project as these old versions are not maintained.
Now we don't support Sphinx < 5, we don't need code that handles older versions, so we can simplify/delete as appropriate.
hoverxref/extension.py
Outdated
# work. This restriction is the lowest version that this extension | ||
# should work on if a user is willing to manually fiddle with dependency | ||
# versions themselves. | ||
app.require_sphinx('3.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure whether to go with what I have here, or just restrict to 5 - happy to change if you prefer to just restrict to 5 (or if we want me to undo other changes to keep theoretical compatibility with much older versions).
Also, I've said '3.0here, but
test_intersphinx_default_configswould fail on Sphinx 3 with my refactor, as it requires 4.0 or newer now I've removed the branch. I think that makes more sense - users fiddling with older versions probably don't care about running the tests here, and when we're running the tests, we'll be obeying the version constraints in
pyproject.toml` in my mind. I can change though if you disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. We can restrict to 5.0 here to match the requirements. Yeah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! (I just amended the previous commit to simplify the git history).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me 馃挴
hoverxref/extension.py
Outdated
# work. This restriction is the lowest version that this extension | ||
# should work on if a user is willing to manually fiddle with dependency | ||
# versions themselves. | ||
app.require_sphinx('3.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. We can restrict to 5.0 here to match the requirements. Yeah.
Great! Let me know if there's anything else you want me to change, otherwise, would you be happy to merge? Then I can rebase #281 on this and get that merged too, then it would be ready for a release restricting to Sphinx 5? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
馃挴
As discussed on #281
As well as the basic changes, I also had a quick search for code branches etc. that dealt with older sphinx versions in particular, and removed them to simplify the code.
Let me know if I'm missing anything!
馃摎 Documentation preview 馃摎: https://sphinx-hoverxref--282.org.readthedocs.build/en/282/