Skip to content
This repository was archived by the owner on Apr 9, 2025. It is now read-only.

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

Merged
merged 10 commits into from
Sep 6, 2024
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
65524c4
Starting sphinx-7.4.0, sphinx.ext.intersphinx is a package, so check …
chrizzFTD Aug 20, 2024
ee8c126
Adding test for disconnecting the intersphinx listener
chrizzFTD Aug 28, 2024
8e38477
removing unused imports
chrizzFTD Aug 28, 2024
ce0d675
adding sphinx 74 to tox
chrizzFTD Aug 28, 2024
102703e
Merge branch 'readthedocs:main' into update_setup_intersphinx_module_…
chrizzFTD Aug 28, 2024
d19ae73
target explicit intersphinx module name depending on sphinx version
chrizzFTD Aug 29, 2024
a242bbf
check for explicit event listeners presence for missing-reference
chrizzFTD Aug 29, 2024
79e07e3
check handler against sphinx_missing_reference explicitly
chrizzFTD Aug 29, 2024
c6eb8af
sphinx.ext.intersphinx.missing_reference continues to be public API i…
chrizzFTD Aug 29, 2024
8d2edf4
Merge branch 'main' into update_setup_intersphinx_module_check
chrizzFTD Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion hoverxref/extension.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ def setup_intersphinx(app, config):

for listener in app.events.listeners.get('missing-reference'):
module_name = inspect.getmodule(listener.handler).__name__
if module_name == 'sphinx.ext.intersphinx':
if module_name.startswith('sphinx.ext.intersphinx'):
Copy link
Member

Choose a reason for hiding this comment

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

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':

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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

app.disconnect(listener.id)


Expand Down
26 changes: 23 additions & 3 deletions tests/test_internals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
import inspect
import os
import pytest
import shutil
from unittest import mock

from .utils import srcdir

Expand Down Expand Up @@ -34,3 +31,26 @@ def _get_docpath(self, builder, docname):
content = open(path).read()

assert app.builder.format == 'latex'


@pytest.mark.sphinx(
srcdir=srcdir,
confoverrides={
'hoverxref_domains': ['py'],
'hoverxref_intersphinx': ['python'],
'hoverxref_auto_ref': True,
'extensions': [
'sphinx.ext.intersphinx',
'hoverxref.extension',
],
},
)
def test_disconnect_intersphinx_listener(app, status, warning):
"""Confirm that disconnecting the ``missing-reference`` listener from ``sphinx.ext.intershinx`` is successful."""
app.build()
listeners = []
for listener in app.events.listeners.get('missing-reference'):
module_name = inspect.getmodule(listener.handler).__name__
if module_name.startswith('sphinx.ext.intersphinx'):
listeners.append((module_name, listener))
assert not listeners, f"Expected to find zero listeners but found: {listeners}"
Copy link
Member

Choose a reason for hiding this comment

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

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

3 changes: 2 additions & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ isolated_build = True

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

[testenv]
deps =
Expand All @@ -18,6 +18,7 @@ deps =
sphinx62: sphinx~=6.2.0
sphinx70: sphinx~=7.0.0
sphinx73: sphinx[test]~=7.3.0
sphinx74: sphinx[test]~=7.4.0
sphinxlatest: sphinx[test]

commands = pytest {posargs}
Expand Down