-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Proposal: add new builder that checks if hardcoded URLs can be replaced with crossrefs #9626
Open
hoefling
wants to merge
23
commits into
sphinx-doc:master
Choose a base branch
from
hoefling:crossrefcheck-builder
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
7511aa5
copy the initial impl from https://github.com/pytest-dev/pytest/pull/…
hoefling eca5a24
drop PEP 585-like type hints
hoefling 7c77805
drop PEP 585-like type hints for real this time, fix wrong module nam…
hoefling 376a4fb
resort imports with isort
hoefling 6f6ff07
move the crossref check into intersphinx extension
hoefling 5071589
refactor initial suggestion, add tests
hoefling 556ae47
replace hardcoded refs in docs with crossrefs
hoefling 6c08ff5
add changelog entry
hoefling e155542
fix docslint
hoefling af58453
fix isort
hoefling 06170a9
apply suggestions from #9800 here as well
hoefling 0312053
fix mypy
hoefling 4e8dea5
fix hardcoded refs to RTD docs
hoefling d35df87
lower checker priority to default one
hoefling 1e28d5e
fix external crossrefs
hoefling b699ce6
chore: invoke isort
hoefling de997b1
chore: invoke ruff
hoefling 51c498f
chore: invoke flake8
hoefling 5efab6f
chore: fix docutils warnings
hoefling 97c7853
chore: invoked isort
hoefling 2d50d64
chore: invoked ruff
hoefling ae2dc24
chore: fix broken type hints
hoefling 0e2ffb9
chore: fix duplicate imports
hoefling File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,8 +116,8 @@ | |
|
||
intersphinx_mapping = { | ||
'python': ('https://docs.python.org/3/', None), | ||
'requests': ('https://requests.readthedocs.io/en/latest/', None), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's minimize the changes (unless there is a reason to change this) |
||
'readthedocs': ('https://docs.readthedocs.io/en/stable', None), | ||
'requests': ('https://requests.readthedocs.io/en/latest/', None), | ||
} | ||
|
||
# Sphinx document translation with sphinx gettext feature uses these settings: | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,7 @@ | |
import sys | ||
import time | ||
from os import path | ||
from typing import TYPE_CHECKING, cast | ||
from typing import IO, TYPE_CHECKING, Any, cast | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where do you need IO and Any as runtime types? |
||
from urllib.parse import urlsplit, urlunsplit | ||
|
||
from docutils import nodes | ||
|
@@ -36,15 +36,15 @@ | |
from sphinx.builders.html import INVENTORY_FILENAME | ||
from sphinx.errors import ExtensionError | ||
from sphinx.locale import _, __ | ||
from sphinx.transforms.post_transforms import ReferencesResolver | ||
from sphinx.transforms.post_transforms import ReferencesResolver, SphinxPostTransform | ||
from sphinx.util import logging, requests | ||
from sphinx.util.docutils import CustomReSTDispatcher, SphinxRole | ||
from sphinx.util.inventory import InventoryFile | ||
|
||
if TYPE_CHECKING: | ||
from collections.abc import Iterable | ||
from collections.abc import Iterable, Iterator | ||
from types import ModuleType | ||
from typing import IO, Any, Union | ||
from typing import Union | ||
|
||
from docutils.nodes import Node, TextElement, system_message | ||
from docutils.utils import Reporter | ||
|
@@ -98,6 +98,77 @@ def clear(self) -> None: | |
self.env.intersphinx_named_inventory.clear() # type: ignore[attr-defined] | ||
|
||
|
||
class ExternalLinksChecker(SphinxPostTransform): | ||
""" | ||
For each external link, check if it can be replaced by a crossreference. | ||
|
||
We treat each ``reference`` node without ``internal`` attribute as an external link. | ||
""" | ||
|
||
default_priority = 500 | ||
|
||
def run(self, **kwargs: Any) -> None: | ||
for refnode in self.document.findall(nodes.reference): | ||
self.check_uri(refnode) | ||
|
||
def check_uri(self, refnode: nodes.reference) -> None: | ||
""" | ||
If the URI in ``refnode`` has a replacement in an ``intersphinx`` inventory, | ||
emit a warning with a replacement suggestion. | ||
""" | ||
if 'internal' in refnode or 'refuri' not in refnode: | ||
return | ||
|
||
uri = refnode['refuri'] | ||
cache = getattr(self.app.env, 'intersphinx_cache', {}) | ||
|
||
for inventory_uri, (inventory_name, _size, _inventory) in cache.items(): | ||
if uri.startswith(inventory_uri): | ||
# build a replacement suggestion | ||
replacements = find_replacements(self.app, uri) | ||
try: | ||
suggestion = f'try using {next(replacements)!r} instead' | ||
logger.warning( | ||
__( | ||
'hardcoded link %r could be replaced by ' | ||
'a cross-reference to %r inventory (%s)', | ||
), | ||
uri, | ||
inventory_name, | ||
suggestion, | ||
location=refnode, | ||
) | ||
except StopIteration: | ||
pass | ||
|
||
|
||
def find_replacements(app: Sphinx, uri: str) -> Iterator[str]: | ||
""" | ||
Try finding a crossreference to replace hardcoded ``uri``. | ||
|
||
This is straightforward: search the available inventories | ||
for an entry that points to the given ``uri`` and build | ||
a ReST markup that should replace ``uri`` with a crossref. | ||
""" | ||
cache = getattr(app.env, 'intersphinx_cache', {}) | ||
|
||
for inventory_uri, (_inventory_name, _size, inventory) in cache.items(): | ||
if uri.startswith(inventory_uri): | ||
for key, entries in inventory.items(): | ||
domain_name, directive_type = key.split(':') | ||
for target, ( | ||
_project_name, | ||
_version, | ||
target_uri, | ||
_display_name, | ||
) in entries.items(): | ||
if uri == target_uri: | ||
for domain in app.env.domains.values(): | ||
if domain_name == domain.name: | ||
role = domain.role_for_objtype(directive_type) or 'any' | ||
yield f':{domain_name}:{role}:`{target}`' | ||
|
||
|
||
def _strip_basic_auth(url: str) -> str: | ||
"""Returns *url* with basic auth credentials removed. Also returns the | ||
basic auth username and password if they're present in *url*. | ||
|
@@ -247,7 +318,7 @@ def fetch_inventory_group( | |
else: | ||
issues = '\n'.join([f[0] % f[1:] for f in failures]) | ||
logger.warning(__("failed to reach any of the inventories " | ||
"with the following issues:") + "\n" + issues) | ||
"with the following issues:") + "\n" + issues) | ||
|
||
|
||
def load_mappings(app: Sphinx) -> None: | ||
|
@@ -691,6 +762,8 @@ def setup(app: Sphinx) -> dict[str, Any]: | |
app.connect('source-read', install_dispatcher) | ||
app.connect('missing-reference', missing_reference) | ||
app.add_post_transform(IntersphinxRoleResolver) | ||
app.add_post_transform(ExternalLinksChecker) | ||
|
||
return { | ||
'version': sphinx.__display_version__, | ||
'env_version': 1, | ||
|
@@ -737,6 +810,7 @@ class MockApp: | |
|
||
if __name__ == '__main__': | ||
import logging as _logging | ||
|
||
_logging.basicConfig() | ||
|
||
raise SystemExit(inspect_main(sys.argv[1:])) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
extensions = ['sphinx.ext.intersphinx'] | ||
intersphinx_mapping = {'python': ('https://example.com', 'inventory.inv')} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
test-ext-intersphinx-hardcoded-urls | ||
=================================== | ||
|
||
.. Links generated by intersphinx crossrefs should not raise any warnings. | ||
.. Only hardcoded URLs are affected. | ||
|
||
:py:mod:`module1` | ||
|
||
.. hardcoded replaceable link | ||
|
||
https://example.com/foo.html#module-module1 | ||
|
||
`inline replaceable link <https://example.com/foo.html#module-module1>`_ | ||
|
||
`replaceable link`_ | ||
|
||
.. hardcoded non-replaceable link | ||
|
||
https://example.com/spam.html#eggs | ||
|
||
`inline non-replaceable link <https://example.com/spam.html#eggs>`_ | ||
|
||
`non-replaceable link`_ | ||
|
||
.. hardcoded external link | ||
|
||
https://spam.eggs | ||
|
||
`inline external link <https://spam.eggs>`_ | ||
|
||
`external link`_ | ||
|
||
.. hyperlinks | ||
|
||
.. _replaceable link: https://example.com/foo.html#module-module1 | ||
.. _non-replaceable link: https://example.com/spam.html#eggs | ||
.. _external link: https://spam.eggs |
5 changes: 5 additions & 0 deletions
5
tests/roots/test-ext-intersphinx-hardcoded-urls/inventory.inv
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This should be moved to the latest version now!