readthedocs / sphinx-hoverxref

Sphinx extension to show tooltips with content embedded when hover a reference.
https://sphinx-hoverxref.readthedocs.io/
MIT License
96 stars 41 forks source link

Update compatibility of setup_intersphinx module name check with Sphinx-7.4 #302

Closed chrizzFTD closed 1 month ago

chrizzFTD commented 2 months ago

Hi hoverxref team,

My tooltips on intersphinx projects stopped working recently. After a while I was able to track down that the break started happening on sphinx-7.4.0.

It seems to be due to sphinx.ext.intersphinx becoming a package since that version (through https://github.com/sphinx-doc/sphinx/pull/12178, undocumented in the changelog), so I've updated setup_intersphinx to check for a prefix in the module name, which should preserve backwards compatibility.

Please let me know your thoughts!

To see in action, the See also links from grill.cook.UsdAsset:

Before this PR With this PR
image image

📚 Documentation preview 📚: https://sphinx-hoverxref--302.org.readthedocs.build/en/302/

humitos commented 2 months ago

Thanks a lot for letting us know about this and opening a PR for it. Would you mind adding a test case for this?

humitos commented 2 months ago

It seems we are not testing for Sphinx 7.4, and that's probably we haven't found this issue. We should update the list from https://github.com/readthedocs/sphinx-hoverxref/blob/80252c3900c657b852c833a4f24b190339473285/tox.ini#L7

chrizzFTD commented 2 months ago

Thanks @humitos ! Will have a look at the test side this week

chrizzFTD commented 2 months ago

Hi @humitos, I've addressed the test and tox points; please let me know your thoughts when you or the team have a chance 😄

chrizzFTD commented 1 month ago

Hi @humitos, while working on improving the test I realized that missing_reference from sphinx.ext.intersphinx was already being imported in the extension module:

https://github.com/readthedocs/sphinx-hoverxref/blob/c6eb8af24259047ef8d2d133c1ad16ee4ee14a08/hoverxref/extension.py#L7

which led me to update the implementation of setup_intersphinx to avoid version checking while guaranteeing the intersphinx listener handler we care about is the one being disconnected:

https://github.com/readthedocs/sphinx-hoverxref/blob/c6eb8af24259047ef8d2d133c1ad16ee4ee14a08/hoverxref/extension.py#L176

I still have the version that does sphinx version check + module name in previous commits, so please let me know your thoughts on whether this update seems reasonable! 😃

chrizzFTD commented 1 month ago

Thanks @humitos !

I can confirm this branch works on my intersphinx project (build here), which installs this branch from git:

Collecting sphinx-hoverxref@ git+https://github.com/chrizzFTD/sphinx-hoverxref.git@update_setup_intersphinx_module_check (from grill==0.17.0)

And can be seen on the two links from grill.cook.UsdAsset's See also section:

image