Update compatibility of setup_intersphinx module name check with Sphinx-7.4

[chrizzFTD]

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 sphinx-doc/sphinx#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

• Chris

---

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

[humitos]

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]

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

sphinx-hoverxref/tox.ini

Line 7 in 80252c3

py{38,39,310,312}-sphinx{50,53,60,62,70,73,latest}

[chrizzFTD]

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

[chrizzFTD]

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

[humitos]

Thanks for updating this PR. It's moving forward in a good direction. I requested some changes, but they are minor.

[humitos]

I found the module_name is called sphinx.ext.intersphinx._resolve. Would it be better to be pretty explicit here to avoid disconnecting things we are not expecting to disconnect?

Suggested change

	if module_name.startswith('sphinx.ext.intersphinx'):
	if module_name == 'sphinx.ext.intersphinx._resolve':

[chrizzFTD]

This explicit name check would be compatible only with Sphinx-7.4+.

We could check explicitly for _resolve first, then the old name as a fallback for backwards compatibility, or according to the Sphinx version. What do you think?

Initially I was moving towards not having a private module in the name check but what you highlight regarding risk of other listeners being disconnected is true, so happy to make the update!

[humitos]

Good point! Yeah, I think it's better to check for the Sphinx version and use one name or the other depending on that 👍🏼

[humitos]

We shouldn't copy the exact code we want to test in the test itself. We should instead test the original function for missing-reference is not present in app.events.listener and the correct one is present.

Pseudo-code for the test:

from sphinx.ext.intersphinx._resolve import missing_reference as intersphinx_missing_reference
from hoverxref.extension import missing_reference

app.build()
assert EventListener(id=Mock.ANY, priority=Mock.ANY, handler=intersphinx_missing_reference) not in app.events.listeners
assert EventListener(id=Mock.ANY, priority=Mock.ANY, handler=missing_reference) in app.events.listeners

[chrizzFTD]

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:

sphinx-hoverxref/hoverxref/extension.py

Line 7 in c6eb8af

from sphinx.ext.intersphinx import missing_reference as sphinx_missing_reference

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:

sphinx-hoverxref/hoverxref/extension.py

Line 176 in c6eb8af

if listener.handler == sphinx_missing_reference:

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! 😃

[humitos]

Looks really good! Thanks a lot for working on this.

Would you mind testing this PR with your documentation to double check it works as expected?

[chrizzFTD]

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: