diff --git a/packages/gcp-sphinx-docfx-yaml/docfx_yaml/extension.py b/packages/gcp-sphinx-docfx-yaml/docfx_yaml/extension.py index 1d8479df1b6b..a934c94f162f 100644 --- a/packages/gcp-sphinx-docfx-yaml/docfx_yaml/extension.py +++ b/packages/gcp-sphinx-docfx-yaml/docfx_yaml/extension.py @@ -29,8 +29,8 @@ import shutil from collections import defaultdict from collections.abc import Mapping, MutableSet, Sequence -from functools import partial -from itertools import zip_longest +from functools import lru_cache, partial +from itertools import chain, zip_longest from pathlib import Path from typing import Any, Iterable @@ -46,13 +46,21 @@ import subprocess import sphinx.application +import yaml from docuploader import shell +from sphinx.builders.html import StandaloneHTMLBuilder from sphinx.errors import ExtensionError from sphinx.ext.napoleon import Config, GoogleDocstring, _process_docstring from sphinx.util import ensuredir from sphinx.util.console import bold, darkgreen from sphinx.util.nodes import make_refnode -from yaml import safe_dump as dump + +try: + from yaml import CSafeDumper as SafeDumper +except ImportError: + from yaml import SafeDumper + +dump = partial(yaml.dump, Dumper=SafeDumper) from docfx_yaml import markdown_utils @@ -184,6 +192,32 @@ def _grab_repo_metadata() -> Mapping[str, str] | None: return None +class DocFXHTMLBuilder(StandaloneHTMLBuilder): + """HTML builder subclass that skips rendering unused HTML pages during DocFX builds.""" + + def write(self, *args: Any, **kwargs: Any) -> None: + pass + + def finish(self) -> None: + pass + + +def _configure_docfx(app: sphinx.application.Sphinx, config: Any) -> None: + """Disables Sphinx extensions from shared conf.py files that DocFX does not need. + + Package conf.py files are shared between HTML docs and DocFX builds and + enable `sphinx.ext.intersphinx` and `sphinx.ext.viewcode` by default. + Neither is used in DocFX YAML output, so we clear `intersphinx_mapping` + (avoiding remote inventory downloads) and disconnect `viewcode` listeners + (avoiding source file tokenization during `doctree-read`). + """ + config.intersphinx_mapping = {} + for listeners in getattr(getattr(app, "events", None), "listeners", {}).values(): + for listener in list(listeners): + if getattr(listener.handler, "__module__", "") == "sphinx.ext.viewcode": + app.disconnect(listener.id) + + def build_init(app: sphinx.application.Sphinx) -> None: """Initializes the build. @@ -197,9 +231,6 @@ def build_init(app: sphinx.application.Sphinx) -> None: else: print("Successfully retrieved repository metadata.") app.env.library_shortname = repo_metadata["name"] - print("Running sphinx-build with Markdown first...") - markdown_utils.run_sphinx_markdown(app) - print("Completed running sphinx-build with Markdown files.") """ Set up environment data @@ -1025,6 +1056,35 @@ def _extract_type_name(annotation: Any) -> str: return type_name +@lru_cache(maxsize=512) +def _get_class_lines(full_path: str) -> dict[str, int]: + """Parses a file once and maps class qualnames to their starting line numbers.""" + lines: dict[str, int] = {} + + def _visit(node: ast.AST, prefix: str = "") -> None: + for child in ast.iter_child_nodes(node): + if isinstance(child, ast.ClassDef): + qual = f"{prefix}{child.name}" + lines.setdefault( + qual, + child.decorator_list[0].lineno + if child.decorator_list + else child.lineno, + ) + _visit(child, f"{qual}.") + elif isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef)): + _visit(child, f"{prefix}{child.name}..") + else: + _visit(child, prefix) + + try: + with open(full_path, "rb") as f: + _visit(ast.parse(f.read())) + except Exception: + pass + return lines + + def _create_datam( app: sphinx.application.Sphinx, cls: str | None, @@ -1180,7 +1240,12 @@ def _update_friendly_package_name(path): # Make relative path = path.replace(os.sep, "", 1) - start_line = inspect.getsourcelines(obj)[1] + unwrapped = inspect.unwrap(obj) + start_line = ( + _get_class_lines(full_path).get(getattr(unwrapped, "__qualname__", ""), 0) + if inspect.isclass(unwrapped) + else 0 + ) or inspect.getsourcelines(obj)[1] path = _update_friendly_package_name(path) @@ -1475,6 +1540,7 @@ def _reformat_pattern(code: str, pattern: str) -> str: return code +@lru_cache(maxsize=4096) def format_code(code: str) -> str: """Reformats code using black.format_str(). @@ -1937,7 +2003,11 @@ def find_uid_to_convert( None if current word does not contain any reference `uid`, or the `uid` that should be converted. """ - for uid in known_uids: + # All Python UIDs are dotted paths (e.g. `pkg.module.Symbol`), so skip + # plain words and sentence-ending periods before scanning `known_uids`. + if "." not in current_word.strip("."): + return None + for uid in chain(known_uids, hard_coded_references or ()): # Do not convert references to itself or containing partial # references. This could result in `storage.types.ReadSession` being # prematurely converted to @@ -1994,6 +2064,19 @@ def convert_cross_references( Returns: content that has been modified with proper cross references if found. """ + # Every UID in `google-*` packages (and in `hard_coded_references`) starts + # with "google.", so if "google." isn't in `content`, no cross-reference can + # match. However, a few packages in the repo don't use the `google.*` + # namespace (e.g. `pandas_gbq.Context`), so we only take this shortcut when + # the package's `known_uids` actually start with "google.". + if ( + known_uids + and known_uids[0].startswith("google.") + and known_uids[-1].startswith("google.") + and "google." not in content + ): + return content + example_text = "Examples:" words = content.split(" ") @@ -2014,7 +2097,6 @@ def convert_cross_references( "google.iam.v1.iam_policy_pb2.TestIamPermissionsResponse": iam_policy_link + "#L120-L131", } - known_uids.extend(hard_coded_references.keys()) # Used to keep track of current position to avoid converting if needed. example_index = len(content) @@ -2267,6 +2349,7 @@ def convert_module_to_package_if_needed(obj): ensuredir(normalized_outdir) # Add markdown pages to the configured output directory. + markdown_utils.run_sphinx_markdown(app) markdown_utils.move_markdown_pages(app, normalized_outdir) pkg_toc_yaml = [] @@ -2277,6 +2360,8 @@ def convert_module_to_package_if_needed(obj): # Used to disambiguate entry names yaml_map = {} + known_uids = sorted(app.env.docfx_uid_names.keys(), reverse=True) + # Order matters here, we need modules before lower level classes, # so that we can make sure to inject the TOC properly for data_set in ( @@ -2433,7 +2518,6 @@ def convert_module_to_package_if_needed(obj): # google.cloud.aiplatform.AutoMLForecastingTrainingJob current_object_name = obj["fullName"] - known_uids = sorted(app.env.docfx_uid_names.keys(), reverse=True) # Currently we only need to look in summary, syntax and # attributes for cross references. search_cross_references(obj, current_object_name, known_uids) @@ -2643,6 +2727,8 @@ def missing_reference( Returns: Any: The new node. """ + if getattr(app.builder, "name", None) == "markdown": + return None reftarget = "" refdoc = "" reftype = "" @@ -2686,6 +2772,8 @@ def setup(app: sphinx.application.Sphinx) -> None: app.add_directive("remarks", RemarksDirective) app.add_directive("todo", TodoDirective) + app.add_builder(DocFXHTMLBuilder, override=True) + app.connect("config-inited", _configure_docfx) app.connect("builder-inited", build_init) app.connect("autodoc-process-docstring", process_docstring) app.connect("autodoc-process-signature", process_signature) diff --git a/packages/gcp-sphinx-docfx-yaml/docfx_yaml/markdown_utils.py b/packages/gcp-sphinx-docfx-yaml/docfx_yaml/markdown_utils.py index 42c018cf77c5..5028eac50e22 100644 --- a/packages/gcp-sphinx-docfx-yaml/docfx_yaml/markdown_utils.py +++ b/packages/gcp-sphinx-docfx-yaml/docfx_yaml/markdown_utils.py @@ -510,26 +510,37 @@ def remove_unused_pages( def run_sphinx_markdown(app: sphinx.application) -> None: - """Runs sphinx-build with Markdown builder in the plugin. + """Runs Markdown builder in-process reusing the already-read Sphinx environment. Args: app (sphinx.application): The sphinx application. """ - cwd = os.getcwd() - relative_srcdir = app.srcdir.removeprefix(f"{cwd}/") - relative_outdir = app.outdir.removeprefix(f"{cwd}/").removesuffix("/html") - # Skip running sphinx-build for Markdown for some unit tests. + # Skip running Markdown builder for some unit tests. # Not required other than to output DocFX YAML. - if "docs" in cwd: + markdown_outdir = Path(app.builder.outdir).parent / "markdown" + if ( + "docs" in os.getcwd() + or markdown_outdir.exists() + or not getattr(app.env, "found_docs", None) + ): return - return shell.run( - [ - "sphinx-build", - "-M", - "markdown", - relative_srcdir, - relative_outdir, - ], - hide_output=False, - ) + from sphinx.util.osutil import ensuredir + from sphinx_markdown_builder.markdown_builder import MarkdownBuilder + + ensuredir(str(markdown_outdir)) + docnames = sorted(app.env.found_docs) + orig_builder = app.builder + md_builder = MarkdownBuilder(app) + md_builder.outdir = str(markdown_outdir) + md_builder.set_environment(app.env) + md_builder.init() + md_builder.prepare_writing(docnames) + app.builder = md_builder + try: + for docname in docnames: + doctree = app.env.get_and_resolve_doctree(docname, md_builder) + md_builder.write_doc_serialized(docname, doctree) + md_builder.write_doc(docname, doctree) + finally: + app.builder = orig_builder diff --git a/packages/gcp-sphinx-docfx-yaml/docfx_yaml/utils.py b/packages/gcp-sphinx-docfx-yaml/docfx_yaml/utils.py index 27e35c799311..967957997d38 100644 --- a/packages/gcp-sphinx-docfx-yaml/docfx_yaml/utils.py +++ b/packages/gcp-sphinx-docfx-yaml/docfx_yaml/utils.py @@ -17,12 +17,16 @@ from inspect import signature from docutils import nodes -from docutils.io import StringOutput +from docutils.frontend import OptionParser from docutils.utils import new_document +from sphinx import addnodes from sphinx.application import Sphinx +from .writer import MarkdownTranslator from .writer import MarkdownWriter as Writer +_DEFAULT_SETTINGS = OptionParser(components=(Writer,)).get_default_values() + def slugify(value: str) -> str: """Converts to lowercase, removes non-word characters. @@ -70,14 +74,18 @@ def transform_node(app: Sphinx, node: nodes.Node) -> str: Returns: str: The transformed node as a string. """ - destination = StringOutput(encoding="utf-8") - doc = new_document(b"") + if node.parent is not None: + node = node.deepcopy() + doc = new_document(b"", _DEFAULT_SETTINGS) doc.append(node) - # Resolve refs + # Resolve refs only when the node actually contains pending cross-references doc["docname"] = "inmemory" - app.env.resolve_references(doctree=doc, fromdocname="inmemory", builder=app.builder) - - writer = Writer(app.builder) - writer.write(doc, destination) - return destination.destination.decode("utf-8") + if any(True for _ in node.traverse(addnodes.pending_xref)): + app.env.resolve_references( + doctree=doc, fromdocname="inmemory", builder=app.builder + ) + + visitor = MarkdownTranslator(doc, app.builder) + doc.walkabout(visitor) + return visitor.body diff --git a/packages/gcp-sphinx-docfx-yaml/tests/test_helpers.py b/packages/gcp-sphinx-docfx-yaml/tests/test_helpers.py index 96ac6c72a0a2..6ddddfdd0d9e 100644 --- a/packages/gcp-sphinx-docfx-yaml/tests/test_helpers.py +++ b/packages/gcp-sphinx-docfx-yaml/tests/test_helpers.py @@ -1,5 +1,6 @@ import tempfile import unittest +import unittest.mock from parameterized import parameterized from yaml import Loader, load @@ -411,6 +412,27 @@ def test_is_not_valid_python_code(self, invalid_syntax): result = extension.is_valid_python_code(invalid_syntax) self.assertFalse(result) + def test_configure_docfx_and_builder(self): + app = unittest.mock.MagicMock() + app.config.intersphinx_mapping = {"python": ("https://example.com", None)} + viewcode_listener = unittest.mock.MagicMock(id=1) + viewcode_listener.handler.__module__ = "sphinx.ext.viewcode" + other_listener = unittest.mock.MagicMock(id=2) + other_listener.handler.__module__ = "docfx_yaml.extension" + app.events.listeners = {"doctree-read": [viewcode_listener, other_listener]} + + extension._configure_docfx(app, app.config) + + self.assertEqual(app.config.intersphinx_mapping, {}) + app.disconnect.assert_called_once_with(1) + self.assertIsNone(extension.DocFXHTMLBuilder.write(None)) + self.assertIsNone(extension.DocFXHTMLBuilder.finish(None)) + + def test_missing_reference_skips_markdown_builder(self): + app = unittest.mock.MagicMock() + app.builder.name = "markdown" + self.assertIsNone(extension.missing_reference(app, None, None, None)) + if __name__ == "__main__": unittest.main() diff --git a/packages/gcp-sphinx-docfx-yaml/tests/test_unit.py b/packages/gcp-sphinx-docfx-yaml/tests/test_unit.py index 99530c4a88d7..9d482518dfb1 100644 --- a/packages/gcp-sphinx-docfx-yaml/tests/test_unit.py +++ b/packages/gcp-sphinx-docfx-yaml/tests/test_unit.py @@ -1,9 +1,14 @@ +import tempfile import unittest +from pathlib import Path +from unittest import mock +from docutils import nodes from parameterized import parameterized +from sphinx import addnodes from yaml import Loader, load -from docfx_yaml import extension +from docfx_yaml import extension, markdown_utils, utils class TestGenerate(unittest.TestCase): @@ -1289,6 +1294,176 @@ def test_merges_markdown_and_package_toc(self): ), ) + def test_get_class_lines(self): + source = ( + "class TopLevel:\n" # line 1 + " pass\n" + "\n" + "@dec1\n" # line 4 + "@dec2\n" # line 5 + "class Decorated:\n" # line 6 + " class Nested:\n" # line 7 + " pass\n" + "\n" + " @nested_dec\n" # line 10 + " class DecoratedNested:\n" # line 11 + " pass\n" + "\n" + "def factory():\n" # line 14 + " class LocalClass:\n" # line 15 + " pass\n" + "\n" + "async def async_factory():\n" # line 18 + " @local_dec\n" # line 19 + " class AsyncLocalClass:\n" # line 20 + " pass\n" + ) + with tempfile.NamedTemporaryFile( + mode="w", suffix=".py", delete=False + ) as tmp_file: + tmp_file.write(source) + tmp_path = tmp_file.name + + try: + extension._get_class_lines.cache_clear() + class_lines = extension._get_class_lines(tmp_path) + self.assertEqual( + class_lines, + { + "TopLevel": 1, + "Decorated": 4, + "Decorated.Nested": 7, + "Decorated.DecoratedNested": 10, + "factory..LocalClass": 15, + "async_factory..AsyncLocalClass": 19, + }, + ) + # Non-existent file returns empty dict without raising. + self.assertEqual( + extension._get_class_lines(tmp_path + ".missing"), + {}, + ) + finally: + Path(tmp_path).unlink(missing_ok=True) + + def test_transform_node_does_not_mutate_live_doctree_or_pending_xref(self): + app = mock.MagicMock() + + def fake_resolve_references(doctree, fromdocname, builder): + for xref in list(doctree.traverse(addnodes.pending_xref)): + ref = nodes.reference("", "", refuri=xref["reftarget"]) + ref.append(nodes.Text(xref.astext())) + xref.replace_self(ref) + + app.env.resolve_references.side_effect = fake_resolve_references + + # 1. Detached node without pending_xref skips resolve_references. + plain_node = nodes.paragraph("", "Plain text") + self.assertEqual(utils.transform_node(app, plain_node).strip(), "Plain text") + app.env.resolve_references.assert_not_called() + + # 2. Live doctree node with nested pending_xref is deep-copied so + # neither its parent pointer nor its nested pending_xref child is mutated. + live_parent = nodes.section() + live_para = nodes.paragraph("", "See ") + live_xref = addnodes.pending_xref( + "", + nodes.literal("", "Client"), + refdomain="py", + reftype="class", + reftarget="google.cloud.Client", + ) + live_para.append(live_xref) + live_parent.append(live_para) + + rendered = utils.transform_node(app, live_para) + + app.env.resolve_references.assert_called_once() + self.assertIs(live_para.parent, live_parent) + self.assertIn(live_xref, live_para.children) + self.assertIs(live_xref.parent, live_para) + self.assertEqual(rendered.strip(), "See ") + + def test_run_sphinx_markdown(self): + with tempfile.TemporaryDirectory() as tmp_dir: + html_outdir = Path(tmp_dir) / "html" + html_outdir.mkdir() + markdown_outdir = Path(tmp_dir) / "markdown" + + orig_builder = mock.MagicMock() + orig_builder.outdir = str(html_outdir) + + app = mock.MagicMock() + app.builder = orig_builder + app.env.found_docs = {"b_doc", "a_doc"} + + doctree_a = mock.sentinel.doctree_a + doctree_b = mock.sentinel.doctree_b + app.env.get_and_resolve_doctree.side_effect = [doctree_a, doctree_b] + + observed_builders = [] + + def record_builder(docname, doctree): + observed_builders.append((docname, doctree, app.builder)) + + with ( + mock.patch("os.getcwd", return_value="/tmp/work"), + mock.patch( + "sphinx_markdown_builder.markdown_builder.MarkdownBuilder" + ) as mock_md_builder_cls, + ): + mock_md_builder = mock_md_builder_cls.return_value + mock_md_builder.write_doc.side_effect = record_builder + + markdown_utils.run_sphinx_markdown(app) + + mock_md_builder_cls.assert_called_once_with(app) + self.assertEqual(mock_md_builder.outdir, str(markdown_outdir)) + mock_md_builder.set_environment.assert_called_once_with(app.env) + mock_md_builder.init.assert_called_once_with() + mock_md_builder.prepare_writing.assert_called_once_with( + ["a_doc", "b_doc"] + ) + self.assertEqual( + observed_builders, + [ + ("a_doc", doctree_a, mock_md_builder), + ("b_doc", doctree_b, mock_md_builder), + ], + ) + # Original builder is restored after run_sphinx_markdown completes. + self.assertIs(app.builder, orig_builder) + + # Subsequent call when markdown_outdir already exists is a no-op. + mock_md_builder_cls.reset_mock() + markdown_utils.run_sphinx_markdown(app) + mock_md_builder_cls.assert_not_called() + + def test_convert_cross_references_fast_path_checks_both_ends(self): + # All UIDs start with "google." and content has no "google." -> fast-path. + all_google_uids = ["google.cloud.storage.Client", "google.api_core.Retry"] + content_no_google = "Plain description with custom_pkg.Client reference." + self.assertEqual( + extension.convert_cross_references( + content_no_google, + "google.cloud.storage.Blob", + all_google_uids, + ), + content_no_google, + ) + + # Mixed sorted list where known_uids[0] starts with "google." but + # known_uids[-1] does not -> must NOT skip non-google references. + mixed_uids = ["google.cloud.storage.Client", "custom_pkg.Client"] + self.assertEqual( + extension.convert_cross_references( + content_no_google, + "google.cloud.storage.Blob", + mixed_uids, + ), + 'Plain description with custom_pkg.Client reference.', + ) + if __name__ == "__main__": unittest.main()