Merge pull request #12 from analog-garage/check-all-xrefs

Check all xrefs (#3)

Author: analog-cbarber

CHANGELOG.md

@@ -1,5 +1,11 @@
 # mkdocstring-python-xref changes
 
+## 1.6.0
+
+* Added explicit option to disable cross-reference checking.
+* When enabled, check all cross-references, not just relative ones
+* If reference begins with '?', don't check cross-reference.
+
 ## 1.5.3
 
 First public release

README.md

@@ -38,6 +38,17 @@ class MyClass:
         """
 ```
 
+Another benefit of this extension is that it will report source locations for bad references
+so that errors are easier to find and fix. For example:
+
+```bash
+$ mkdocs build
+INFO    -  Cleaning site directory
+INFO    -  Building documentation to directory: /home/jdoe/my-project/site
+WARNING -  mkdocstrings_handlers: file:///home/jdoe/my-project/src/myproj/bar.py:16:
+           Cannot load reference 'myproj.bar.bad'
+```
+
 For further details, please see the [Documentation](https://analog-garage.github.io/mkdocstrings-python-xref/)
 
 [MkDocs]: https://mkdocs.readthedocs.io/

docs/config.md

@@ -3,10 +3,16 @@ that the handler name should be `python_xref` instead of `python`. Because
 this handler extends the standard [mkdocstrings-python][] handler, the same options are
 available.
 
-Additional options are added by this extension. Currently, there is only one:
+Additional options are added by this extension. Currently, there are two:
 
 * **relative_crossrefs** - if set to true enables use of relative path syntax in
     cross-references.
+    
+* **check_crossrefs** - enables early checking of all cross-references. Note that
+    this option only takes affect if **relative_crossrefs** is also true. This option is
+    true by default, so this option is used to disable checking. Checking can
+    also be disabled on a per-case basis by prefixing the reference with '?', e.g.
+    `[something][?dontcheckme]`.
 
 !!! Example "mkdocs.yml plugins specification using this handler"
 
@@ -25,6 +31,7 @@ plugins:
             ignore_init_summary: yes
           merge_init_into_class: yes
           relative_crossrefs: yes
+          check_crossrefs: no
           separate_signature: yes
           show_source: no
           show_root_full_path: no

docs/index.md

@@ -101,6 +101,27 @@ These are demonstrated here:
 This has been [proposed as a feature in the standard python handler][relative-crossref-issue]
 but has not yet been accepted.
 
+## Cross-reference checking
+
+If `relative_crossrefs` and `check_crossrefs` are both enabled (the latter is true by default),
+then all cross-reference expressions will be checked to ensure that they exist and failures
+will be reported with the source location. Otherwise, missing cross-references will be reported
+by mkdocstrings without the source location, in which case it is often difficult to locate the source
+of the error. Note that the errors generatoed by this feat[.gitignore](..%2F.gitignore)
+
+
+
+ure are in addition to the errors
+from mkdocstrings.
+
+The current implementation of this feature can produce false errors for definitions from the
+python standard library. You can disable the check on a case-by-case basis by prefixing the
+reference expression with a `?`, for example:
+
+```
+This function returns a [Path][?pathlib.] instance.
+```
+
 [mkdocstrings]: https://mkdocstrings.github.io/
 [mkdocstrings_python]: https://mkdocstrings.github.io/python/
 [relative-crossref-issue]: https://github.com/mkdocstrings/python/issues/27

src/mkdocstrings_handlers/python_xref/VERSION

@@ -1,3 +1 @@
-1.5.3
-
-
+1.6.0

src/mkdocstrings_handlers/python_xref/crossref.py

@@ -17,7 +17,7 @@
 
 import re
 import sys
-from typing import Callable, List, Optional, Union, cast
+from typing import Callable, List, Optional, cast
 
 from griffe.dataclasses import Docstring, Object
 from mkdocstrings.loggers import get_logger
@@ -57,8 +57,10 @@ def _re_named(name: str, exp: str, optional: bool = False) -> str:
     optchar = "?" if optional else ""
     return f"(?P<{name}>{exp}){optchar}"
 
+_RE_CROSSREF = re.compile(r"\[([^\[\]]+?)\]\[([^\[\]]*?)\]")
+"""Regular expression that matches general cross-references."""
 
-_RE_REL_CROSSREF = re.compile(r"\[([^\[\]]+?)\]\[([\.^\(][^\]]*?|[^\]]*?\.)\]")
+_RE_REL_CROSSREF = re.compile(r"\[([^\[\]]+?)\]\[(\??(?:[\.^\(][^\]]*?|[^\]]*?\.))\]")
 """Regular expression that matches relative cross-reference expressions in doc-string.
 
 This will match a cross reference where the path expression either ends in '.'
@@ -98,13 +100,13 @@ def _re_named(name: str, exp: str, optional: bool = False) -> str:
 """Regular expression that matches a qualified python identifier."""
 
 
-def _always_ok(_ref:str) -> bool:
+def _always_ok(_ref: str) -> bool:
     return True
 
 
 class _RelativeCrossrefProcessor:
     """
-    A callable object that substitutes relative cross-reference expressions.
+    A callable object that can substitute relative cross-reference expressions.
 
     This is intended to be used as a substitution function by `re.sub`
     to process relative cross-references in a doc-string.
@@ -116,7 +118,7 @@ class _RelativeCrossrefProcessor:
     _cur_offset: int
     _cur_ref_parts: List[str]
     _ok: bool
-    _check_ref: Union[Callable[[str],bool],Callable[[str],bool]]
+    _check_ref: Callable[[str], bool]
 
     def __init__(self, doc: Docstring, checkref: Optional[Callable[[str], bool]] = None):
         self._doc = doc
@@ -128,27 +130,52 @@ def __init__(self, doc: Docstring, checkref: Optional[Callable[[str], bool]] = N
         self._ok = True
 
     def __call__(self, match: re.Match) -> str:
+        """
+        Process a cross-reference expression.
+
+        This should be called with a match from the _RE_CROSSREF expression
+        which matches expression of the form [<title>][<ref>].
+        Group 1 matches the <title> and 2 the <ref>.
+        """
         self._start_match(match)
 
         title = match[1]
         ref = match[2]
 
-        ref_match = _RE_REL.fullmatch(ref)
-        if ref_match is None:
-            self._error(f"Bad syntax in relative cross reference: '{ref}'")
+        checkref = self._check_ref
+        if ref.startswith("?"):
+            # Turn off cross-ref check
+            ref = ref[1:]
+            checkref = _always_ok
+
+        new_ref = ""
+
+        # TODO support special syntax to turn off checking
+
+        if not _RE_REL_CROSSREF.fullmatch(match.group(0)):
+            # Just a regular cross reference
+            new_ref = ref if ref else title
         else:
-            self._process_parent_specifier(ref_match)
-            self._process_relname(ref_match)
-            self._process_append_from_title(ref_match, title)
-
-        if self._ok:
-            new_ref = '.'.join(self._cur_ref_parts)
-            logger.debug(
-                "cross-reference substitution\nin %s:\n[%s][%s] -> [...][%s]",
-                cast(Object,self._doc.parent).canonical_path, title, ref, new_ref
-            )
-            if not self._check_ref(new_ref):
-                self._error(f"Cannot load reference '{new_ref}'")
+            ref_match = _RE_REL.fullmatch(ref)
+            if ref_match is None:
+                self._error(f"Bad syntax in relative cross reference: '{ref}'")
+            else:
+                self._process_parent_specifier(ref_match)
+                self._process_relname(ref_match)
+                self._process_append_from_title(ref_match, title)
+
+            if self._ok:
+                new_ref = '.'.join(self._cur_ref_parts)
+                logger.debug(
+                    "cross-reference substitution\nin %s:\n[%s][%s] -> [...][%s]",
+                    cast(Object, self._doc.parent).canonical_path, title, ref, new_ref
+                )
+
+        # builtin names get handled specially somehow, so don't check here
+        if new_ref not in __builtins__ and not checkref(new_ref):  # type: ignore[operator]
+            self._error(f"Cannot load reference '{new_ref}'")
+
+        if new_ref:
             result = f"[{title}][{new_ref}]"
         else:
             result = match.group(0)
@@ -265,7 +292,7 @@ def _process_up_specifier(self, obj: Object, ref_match: re.Match) -> Optional[Ob
                     break
         return rel_obj
 
-    def _error(self, msg: str, just_warn:bool = False) -> None:
+    def _error(self, msg: str, just_warn: bool = False) -> None:
         """Logs a warning for a specific crossref in a docstring.
 
         This will include the filepath and line number if available.
@@ -281,8 +308,8 @@ def _error(self, msg: str, just_warn:bool = False) -> None:
             # recognize that this is a navigable location it can highlight.
             prefix = f"file://{parent.filepath}:"
             line = doc.lineno
-            if line is not None: # pragma: no branch
-                if _supports_linenums: # pragma: no branch
+            if line is not None:  # pragma: no branch
+                if _supports_linenums:  # pragma: no branch
                     # Add line offset to match in docstring. This can still be
                     # short if the doc string has leading newlines.
                     line += doc.value.count("\n", 0, self._cur_offset)
@@ -296,7 +323,7 @@ def _error(self, msg: str, just_warn:bool = False) -> None:
         self._ok = just_warn
 
 
-def substitute_relative_crossrefs(obj: Object, checkref: Optional[Callable[[str],bool]] = None) -> None:
+def substitute_relative_crossrefs(obj: Object, checkref: Optional[Callable[[str], bool]] = None) -> None:
     """Recursively expand relative cross-references in all docstrings in tree.
 
     Arguments:
@@ -307,9 +334,8 @@ def substitute_relative_crossrefs(obj: Object, checkref: Optional[Callable[[str]
     doc = obj.docstring
 
     if doc is not None:
-        doc.value = _RE_REL_CROSSREF.sub(_RelativeCrossrefProcessor(doc, checkref=checkref), doc.value)
+        doc.value = _RE_CROSSREF.sub(_RelativeCrossrefProcessor(doc, checkref=checkref), doc.value)
 
     for member in obj.members.values():
-        if isinstance(member, Object): # pragma: no branch
-            substitute_relative_crossrefs(member, checkref= checkref)
-
+        if isinstance(member, Object):  # pragma: no branch
+            substitute_relative_crossrefs(member, checkref=checkref)

src/mkdocstrings_handlers/python_xref/handler.py

@@ -12,7 +12,7 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 """
-Implementation of GarpyPythonHandler
+Implementation of python_xref handler
 """
 
 from __future__ import annotations
@@ -35,13 +35,17 @@
 
 class PythonRelXRefHandler(PythonHandler):
     """Extended version of mkdocstrings Python handler
+
+    * Converts relative cross-references into full references
+    * Checks cross-references early in order to produce errors with source location
     """
 
     handler_name: str = __name__.rsplit('.', 2)[1]
 
     default_config = dict(
         PythonHandler.default_config,
         relative_crossrefs = False,
+        check_crossrefs = True,
     )
 
     def __init__(self,
@@ -65,9 +69,14 @@ def render(self, data: Object, config: Mapping[str,Any]) -> str:
         final_config = ChainMap(config, self.default_config) # type: ignore[arg-type]
 
         if final_config["relative_crossrefs"]:
-            substitute_relative_crossrefs(data, checkref=self._check_ref)
+            checkref = self._check_ref if final_config["check_crossrefs"] else None
+            substitute_relative_crossrefs(data, checkref=checkref)
 
-        return super().render(data, config)
+        try:
+            return super().render(data, config)
+        except Exception:  # pragma: no cover
+            print(f"{data.path=}")
+            raise
 
     def get_templates_dir(self, handler: Optional[str] = None) -> Path:
         """See [render][.barf]"""

tests/test_crossref.py

@@ -27,6 +27,7 @@
 # noinspection PyProtectedMember
 import mkdocstrings_handlers.python_xref.crossref
 from mkdocstrings_handlers.python_xref.crossref import (
+    _RE_CROSSREF,
     _RE_REL_CROSSREF,
     _RelativeCrossrefProcessor,
     substitute_relative_crossrefs,
@@ -50,7 +51,9 @@ def test_RelativeCrossrefProcessor(caplog: pytest.LogCaptureFixture, monkeypatch
 
     def assert_sub(parent: Object, title: str, ref: str,
                    expected: str = "",
+                   *,
                    warning: str = "",
+                   relative: bool = True,
                    checkref: Optional[Callable[[str],bool]] = None
                    ) -> None:
         """Tests a relative crossref substitution
@@ -61,14 +64,20 @@ def assert_sub(parent: Object, title: str, ref: str,
             ref: the reference path section of the cross-reference expression
             expected: the expected new value for the cross-reference
             warning: if specified, is regexp matching expected warning message
+            relative: true if relative reference is expected
             checkref: reference checking function
         """
         if not expected:
             expected = ref
         crossref = f"[{title}][{ref}]"
         doc = Docstring(parent=parent, value=f"subject\n\n{crossref}\n", lineno=42)
         match = _RE_REL_CROSSREF.search(doc.value)
-        assert match is not None
+        if relative:
+            assert match is not None
+        else:
+            assert match is None
+            match = _RE_CROSSREF.search(doc.value)
+            assert match is not None
         caplog.clear()
         actual = _RelativeCrossrefProcessor(doc, checkref=checkref)(match)
         if warning:
@@ -97,6 +106,16 @@ def assert_sub(parent: Object, title: str, ref: str,
     assert_sub(meth1, "Class1", "(p).mod2.", "mod1.mod2.Class1")
     assert_sub(mod1, "Class1", "(P).mod2.Class1", "mod1.mod2.Class1")
 
+    # disable checking
+
+    def assert_nocheck(val: str) -> bool:
+        pytest.fail(f"unexpected check of '{val}'")
+        return False
+
+    assert_sub(cls1, "foo", "?.", "mod1.mod2.Class1.foo", checkref=assert_nocheck)
+    assert_sub(cls1, "foo", "?mod1.mod2.Class1.foo", "mod1.mod2.Class1.foo",
+               checkref=assert_nocheck, relative=False)
+
     # Error cases
 
     assert_sub(meth1, "foo", ".", ".", warning="Cannot use '.'")
@@ -108,6 +127,10 @@ def assert_sub(parent: Object, title: str, ref: str,
     assert_sub(meth1, "foo", "..", "mod1.mod2.Class1.foo",
                warning = "Cannot load reference 'mod1.mod2.Class1.foo'",
                checkref=lambda x: False)
+    assert_sub(meth1, "foo", "mod1.mod2.Class1.foo", "mod1.mod2.Class1.foo",
+               warning = "Cannot load reference 'mod1.mod2.Class1.foo'",
+               relative=False,
+               checkref=lambda x: False)
 
 
 def test_substitute_relative_crossrefs(caplog: pytest.LogCaptureFixture) -> None:

tests/test_handler.py

@@ -80,6 +80,7 @@ def fake_render(_self: PythonHandler, data: Object, _config: dict) -> str:
         assert data.docstring is not None
         return data.docstring.value
 
+    # Monkeypatch render/collect methods on parent class
     monkeypatch.setattr(PythonHandler, 'collect', fake_collect)
     monkeypatch.setattr(PythonHandler, 'render', fake_render)
 
@@ -95,9 +96,32 @@ def fake_render(_self: PythonHandler, data: Object, _config: dict) -> str:
 
     rendered = handler.render(obj, dict(relative_crossrefs=True))
     assert rendered == "[foo][mod.foo] [bar][bad.bar]"
+    assert len(caplog.records) == 1
+    _, level, msg = caplog.record_tuples[0]
+    assert level == logging.WARNING
+    assert "Cannot load reference 'bad.bar'" in msg
+    caplog.clear()
+
+    rendered = handler.render(obj, dict(relative_crossrefs=True, check_crossrefs=False))
+    assert rendered == "[foo][mod.foo] [bar][bad.bar]"
+    assert len(caplog.records) == 0
 
+    rendered = handler.render(obj, dict(relative_crossrefs=True, check_crossrefs=False))
+    assert rendered == "[foo][mod.foo] [bar][bad.bar]"
+    assert len(caplog.records) == 0
+
+    docstring = "\n\n[foo][bad.foo]"
+    obj.docstring = Docstring(docstring, parent=obj)
+    rendered = handler.render(obj, dict(relative_crossrefs=True))
+    assert rendered == "[foo][bad.foo]"
     assert len(caplog.records) == 1
     _, level, msg = caplog.record_tuples[0]
     assert level == logging.WARNING
-    assert 'Cannot load reference' in msg
+    assert "Cannot load reference 'bad.foo'" in msg
+    caplog.clear()
 
+    docstring = "[foo][?bad.foo] [bar][?bad.]"
+    obj.docstring = Docstring(docstring, parent=obj)
+    rendered = handler.render(obj, dict(relative_crossrefs=True, check_crossrefs=True))
+    assert rendered == "[foo][bad.foo] [bar][bad.bar]"
+    assert len(caplog.records) == 0