From 333809348508d16df9562208504af38000a18c0f Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 15 Apr 2020 21:28:03 +0200 Subject: [PATCH 1/8] Translator on current builder We set our own translator in the translators defined by other extensions, but also for the current builder. It may happen that there is an extension that defines two translators for `dirhml` and `json`, and we are going to override them --the problem comes when `sphinx-build` is called with `-b custombuilder` since we weren't overriding it in that case. This commit override the ones defined by the user, but also the default one. --- hoverxref/extension.py | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/hoverxref/extension.py b/hoverxref/extension.py index 8ee4e576..5ee648ee 100644 --- a/hoverxref/extension.py +++ b/hoverxref/extension.py @@ -147,31 +147,31 @@ def setup_translators(app): and our own ``HoverXRefHTMLTranslatorMixin`` that includes the logic to ``_hoverxref`` attributes. """ - if not app.registry.translators.items(): + for name, klass in app.registry.translators.items(): + if app.builder.format != 'html': + # Skip translators that are not HTML + continue + translator = types.new_class( 'HoverXRefHTMLTranslator', ( HoverXRefHTMLTranslatorMixin, - app.builder.default_translator_class, + klass, ), {}, ) - app.set_translator(app.builder.name, translator, override=True) - else: - for name, klass in app.registry.translators.items(): - if app.builder.format != 'html': - # Skip translators that are not HTML - continue - - translator = types.new_class( - 'HoverXRefHTMLTranslator', - ( - HoverXRefHTMLTranslatorMixin, - klass, - ), - {}, - ) - app.set_translator(name, translator, override=True) + app.set_translator(name, translator, override=True) + + translator = types.new_class( + 'HoverXRefHTMLTranslator', + ( + HoverXRefHTMLTranslatorMixin, + app.builder.default_translator_class, + ), + {}, + ) + app.set_translator(app.builder.name, translator, override=True) + def is_hoverxref_configured(app, config): From 3dd5eaf563960a6bbd613968186e0aa163c02cf3 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 16 Apr 2020 13:48:55 +0200 Subject: [PATCH 2/8] Refactor tests to share functionability --- tests/__init__.py | 0 tests/conftest.py | 14 ++++++++++++++ tests/test_htmltag.py | 39 +-------------------------------------- tests/utils.py | 29 +++++++++++++++++++++++++++++ 4 files changed, 44 insertions(+), 38 deletions(-) create mode 100644 tests/__init__.py create mode 100644 tests/conftest.py create mode 100644 tests/utils.py diff --git a/tests/__init__.py b/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 00000000..15841b8d --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,14 @@ +import os +import shutil +import pytest + +from .utils import srcdir, prefixdocumentsrcdir, customobjectsrcdir, pythondomainsrcdir + + +@pytest.fixture(autouse=True, scope='function') +def remove_sphinx_build_output(): + """Remove _build/ folder, if exist.""" + for path in (srcdir, prefixdocumentsrcdir, customobjectsrcdir, pythondomainsrcdir): + build_path = os.path.join(path, '_build') + if os.path.exists(build_path): + shutil.rmtree(build_path) diff --git a/tests/test_htmltag.py b/tests/test_htmltag.py index 340234a2..6077d369 100644 --- a/tests/test_htmltag.py +++ b/tests/test_htmltag.py @@ -1,43 +1,6 @@ -import os import pytest -import shutil - -srcdir = os.path.join( - os.path.dirname(os.path.abspath(__file__)), - 'examples', - 'default', -) - -# srcdir with ``autosectionlabel_prefix_document = True`` config -prefixdocumentsrcdir = os.path.join( - os.path.dirname(os.path.abspath(__file__)), - 'examples', - 'prefixdocument', -) - -# srcdir with ``Sphinx.add_object_type`` call -customobjectsrcdir = os.path.join( - os.path.dirname(os.path.abspath(__file__)), - 'examples', - 'custom-object', -) - -# srcdir with ``:py:class:`` call -pythondomainsrcdir = os.path.join( - os.path.dirname(os.path.abspath(__file__)), - 'examples', - 'python-domain', -) - - -@pytest.fixture(autouse=True, scope='function') -def remove_sphinx_build_output(): - """Remove _build/ folder, if exist.""" - for path in (srcdir, prefixdocumentsrcdir, customobjectsrcdir, pythondomainsrcdir): - build_path = os.path.join(path, '_build') - if os.path.exists(build_path): - shutil.rmtree(build_path) +from .utils import srcdir, prefixdocumentsrcdir, customobjectsrcdir, pythondomainsrcdir @pytest.mark.sphinx( diff --git a/tests/utils.py b/tests/utils.py new file mode 100644 index 00000000..a2c609ab --- /dev/null +++ b/tests/utils.py @@ -0,0 +1,29 @@ +import os + + +srcdir = os.path.join( + os.path.dirname(os.path.abspath(__file__)), + 'examples', + 'default', +) + +# srcdir with ``autosectionlabel_prefix_document = True`` config +prefixdocumentsrcdir = os.path.join( + os.path.dirname(os.path.abspath(__file__)), + 'examples', + 'prefixdocument', +) + +# srcdir with ``Sphinx.add_object_type`` call +customobjectsrcdir = os.path.join( + os.path.dirname(os.path.abspath(__file__)), + 'examples', + 'custom-object', +) + +# srcdir with ``:py:class:`` call +pythondomainsrcdir = os.path.join( + os.path.dirname(os.path.abspath(__file__)), + 'examples', + 'python-domain', +) From 1d238d4a1239f8ac8dcb4d1ac16d64fd8fe3c04a Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 16 Apr 2020 13:49:22 +0200 Subject: [PATCH 3/8] Do not override Translators if the builder is non-html --- hoverxref/extension.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/hoverxref/extension.py b/hoverxref/extension.py index 5ee648ee..d2f2b5aa 100644 --- a/hoverxref/extension.py +++ b/hoverxref/extension.py @@ -147,11 +147,12 @@ def setup_translators(app): and our own ``HoverXRefHTMLTranslatorMixin`` that includes the logic to ``_hoverxref`` attributes. """ - for name, klass in app.registry.translators.items(): - if app.builder.format != 'html': - # Skip translators that are not HTML - continue + if app.builder.format != 'html': + # do not modify non-html builders + return + + for name, klass in app.registry.translators.items(): translator = types.new_class( 'HoverXRefHTMLTranslator', ( From 7a1d9e4d2a2175febd6c728e4877b347950f19bf Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 16 Apr 2020 13:49:36 +0200 Subject: [PATCH 4/8] Tests for Translator overrides --- tests/test_internals.py | 46 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 tests/test_internals.py diff --git a/tests/test_internals.py b/tests/test_internals.py new file mode 100644 index 00000000..56f7ecdb --- /dev/null +++ b/tests/test_internals.py @@ -0,0 +1,46 @@ +import inspect +import os +import pytest +import shutil + +from hoverxref.translators import HoverXRefHTMLTranslatorMixin + +from .utils import srcdir + + +@pytest.mark.sphinx( + srcdir=srcdir, + buildername='latex', + confoverrides={ + 'hoverxref_project': 'myproject', + 'hoverxref_version': 'myversion', + }, +) +def test_dont_override_translator_non_html_builder(app, status, warning): + app.build() + path = app.outdir / 'python.tex' + assert path.exists() is True + content = open(path).read() + + assert app.builder.format == 'latex' + for name, klass in app.registry.translators.items(): + assert not issubclass(klass, HoverXRefHTMLTranslatorMixin) + + +@pytest.mark.sphinx( + srcdir=srcdir, + buildername='html', + confoverrides={ + 'hoverxref_project': 'myproject', + 'hoverxref_version': 'myversion', + }, +) +def test_override_translator_non_html_builder(app, status, warning): + app.build() + path = app.outdir / 'index.html' + assert path.exists() is True + content = open(path).read() + + assert app.builder.format == 'html' + for name, klass in app.registry.translators.items(): + assert issubclass(klass, HoverXRefHTMLTranslatorMixin) From 06555617d3b367cc5e8f5800ba2042f2fc54dae7 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 16 Apr 2020 14:27:26 +0200 Subject: [PATCH 5/8] Force to use always the same .tex filename output It differs in Sphinx1.8 and the rest. So, forcing its name to be always the same in tests. --- tests/examples/default/conf.py | 5 +++++ tests/test_internals.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/examples/default/conf.py b/tests/examples/default/conf.py index 6eb08bcb..4e612d00 100644 --- a/tests/examples/default/conf.py +++ b/tests/examples/default/conf.py @@ -5,3 +5,8 @@ 'sphinx.ext.autosectionlabel', 'hoverxref.extension', ] + +latex_documents = [ + (master_doc, 'test.tex', u'test Documentation', + u'test', 'manual'), +] diff --git a/tests/test_internals.py b/tests/test_internals.py index 56f7ecdb..5df069e5 100644 --- a/tests/test_internals.py +++ b/tests/test_internals.py @@ -18,7 +18,7 @@ ) def test_dont_override_translator_non_html_builder(app, status, warning): app.build() - path = app.outdir / 'python.tex' + path = app.outdir / 'test.tex' assert path.exists() is True content = open(path).read() From 45696570cc740aac5b73badfa56105be0a5d5dde Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 16 Apr 2020 15:34:02 +0200 Subject: [PATCH 6/8] Ignore non-HTML builders when resolving references It does not make sense to override the domain classes when using other non-HTML builders because there is nothing to add. Although, due to a initialization race condition, we are not able to "not override the Domains if the builder is non-HTML" so we need to skip our custom resolution during build time. --- hoverxref/domains.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/hoverxref/domains.py b/hoverxref/domains.py index c70af5c7..f8c36515 100644 --- a/hoverxref/domains.py +++ b/hoverxref/domains.py @@ -52,6 +52,16 @@ def _get_docpath(self, builder, docname): return docpath def _is_ignored_ref(self, env, target): + # HACK: skip all references if the builder is non-html. We shouldn't + # have overridden the Domain in first instance at ``setup_domains`` + # function, but at that time ``app.builder`` is not yet initialized. If + # we suscribe ourselves to ``builder-initied`` it's too late and our + # override does not take effect. Other builders (e.g. LatexBuilder) may + # fail with internal functions we use (e.g. builder.get_outfilename). + # So, we are skipping it here :( + if env.app.builder.format != 'html': + return True + if target in env.config.hoverxref_ignore_refs: logger.info( 'Ignoring reference in hoverxref_ignore_refs. target=%s', From 561f5c27bcbf761e6f80b5726ab558f4eb53284a Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 16 Apr 2020 17:56:47 +0200 Subject: [PATCH 7/8] Test that do not fail when running LaTeX builder with hoverxref_auto_ref=True --- tests/examples/default/index.rst | 7 +++++++ tests/test_internals.py | 30 ++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/tests/examples/default/index.rst b/tests/examples/default/index.rst index d528f10a..4461a286 100644 --- a/tests/examples/default/index.rst +++ b/tests/examples/default/index.rst @@ -9,3 +9,10 @@ Using ``hoverxref`` (or ``ref`` if ``hoverxref_auto_ref=True``) should add an `` :ref:`This a :ref: to Chapter I `. :hoverxref:`This a :hoverxref: to Chapter I, Section I
`. + +.. _example-reference: + +Example Reference +----------------- + +This is a reference to :ref:`example-reference`. diff --git a/tests/test_internals.py b/tests/test_internals.py index 5df069e5..ee5cb06d 100644 --- a/tests/test_internals.py +++ b/tests/test_internals.py @@ -44,3 +44,33 @@ def test_override_translator_non_html_builder(app, status, warning): assert app.builder.format == 'html' for name, klass in app.registry.translators.items(): assert issubclass(klass, HoverXRefHTMLTranslatorMixin) + + +@pytest.mark.sphinx( + srcdir=srcdir, + buildername='latex', + confoverrides={ + 'hoverxref_project': 'myproject', + 'hoverxref_version': 'myversion', + 'hoverxref_auto_ref': True, + }, +) +def test_dont_fail_non_html_builder(app, status, warning): + """ + Test our resolver is not used by non-HTML builder. + + When running the build with ``latex`` as builder and + ``hoverxref_auto_ref=True`` it should not fail with + + def _get_docpath(self, builder, docname): + docpath = builder.get_outfilename(docname) + AttributeError: 'LaTeXBuilder' object has no attribute 'get_outfilename' + + LaTeXBuilder should never use our resolver. + """ + app.build() + path = app.outdir / 'test.tex' + assert path.exists() is True + content = open(path).read() + + assert app.builder.format == 'latex' From 283c6be70f8f939f29efa67755ee1c17589f25db Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Thu, 16 Apr 2020 18:03:02 +0200 Subject: [PATCH 8/8] Check the method is not called --- tests/test_internals.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/test_internals.py b/tests/test_internals.py index ee5cb06d..d031a88e 100644 --- a/tests/test_internals.py +++ b/tests/test_internals.py @@ -2,6 +2,7 @@ import os import pytest import shutil +from unittest import mock from hoverxref.translators import HoverXRefHTMLTranslatorMixin @@ -68,7 +69,10 @@ def _get_docpath(self, builder, docname): LaTeXBuilder should never use our resolver. """ - app.build() + + with mock.patch('hoverxref.domains.HoverXRefBaseDomain._get_docpath') as _get_docpath: + app.build() + assert not _get_docpath.called path = app.outdir / 'test.tex' assert path.exists() is True content = open(path).read()