From 850f7c90b92a9daf3867bef7dfc2f58d6b963aed Mon Sep 17 00:00:00 2001 From: Chris Sewell Date: Mon, 5 Aug 2024 15:03:10 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=91=8C=20Improve=20footnote=20def/ref=20w?= =?UTF-8?q?arnings=20and=20translations=20(#931)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Footnotes are now parsed similar to the corresponding restructuredtext, in that resolution (between definitions and references) and ordering is now deferred to transforms on the doctree. This allows for the proper interaction with other docutils/sphinx transforms, including those that perform translations. In addition, an upstream improvement to unreferenced footnote definitions is also added here: https://github.com/sphinx-doc/sphinx/pull/12730, so that unreferenced and duplicate definitions are correctly warned about, e.g.: ``` source/index.md:1: WARNING: Footnote [1] is not referenced. [ref.footnote] source/index.md:4: WARNING: Duplicate footnote definition found for label: 'a' [ref.footnote] ``` It is of note that warnings for references with no corresponding definitions are deferred to docutils to handle, e.g. for `[^a]` with no definition: ``` source/index.md:1: ERROR: Too many autonumbered footnote references: only 0 corresponding footnotes available. [docutils] source/index.md:1: ERROR: Unknown target name: "a". [docutils] ``` These warning messages are a little obscure, and it would be ideal that one clear warning was emitted for the issue. However, it is non-trivial in this extension; to both suppress the existing warnings, and then replace them with a better one, so for now we don't do it here, and ideally this would be improved upstream in docutils. --- docs/configuration.md | 4 +- docs/syntax/typography.md | 19 +- myst_parser/config/main.py | 10 +- myst_parser/mdit_to_docutils/base.py | 89 +++--- myst_parser/mdit_to_docutils/transforms.py | 127 ++++++++- myst_parser/parsers/docutils_.py | 14 +- myst_parser/parsers/mdit.py | 5 +- myst_parser/parsers/sphinx_.py | 12 +- myst_parser/sphinx_ext/main.py | 10 + myst_parser/warnings_.py | 41 +-- pyproject.toml | 2 +- .../fixtures/docutil_syntax_elements.md | 18 +- .../fixtures/reporter_warnings.md | 24 +- .../fixtures/sphinx_syntax_elements.md | 18 +- .../test_renderers/test_fixtures_docutils.py | 14 +- tests/test_renderers/test_fixtures_sphinx.py | 53 ++-- .../test_sphinx/sourcedirs/footnotes/conf.py | 1 + .../sourcedirs/footnotes/footnote_md.md | 28 +- .../sourcedirs/footnotes/footnote_rst.rst | 4 +- .../gettext/fr/LC_MESSAGES/index.po | 12 + tests/test_sphinx/sourcedirs/gettext/index.md | 6 + tests/test_sphinx/test_sphinx_builds.py | 18 +- .../test_sphinx_builds/test_footnotes.html | 262 ++++++++++-------- .../test_sphinx_builds/test_footnotes.xml | 101 ++++--- .../test_sphinx_builds/test_gettext.pot | 12 + .../test_gettext_additional_targets.pot | 12 + .../test_sphinx_builds/test_gettext_html.html | 56 ++++ .../test_gettext_html.resolved.xml | 18 ++ .../test_sphinx_builds/test_gettext_html.xml | 18 ++ tox.ini | 7 +- 30 files changed, 714 insertions(+), 301 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index fc00df9f..6f4d7790 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -195,13 +195,13 @@ tasklist (myst-warnings)= ## Build Warnings -Below lists the MyST specific warnings that may be emitted during the build process. These will be prepended to the end of the warning message, e.g. +Below lists the MyST specific warnings that may be emitted during the build process. These will be prepended to the end of the warning message (see also ), e.g. ``` WARNING: Non-consecutive header level increase; H1 to H3 [myst.header] ``` -**In general, if your build logs any warnings, you should either fix them or [raise an Issue](https://github.com/executablebooks/MyST-Parser/issues/new/choose) if you think the warning is erroneous.** +In general, if your build logs any warnings, you should either fix them or [raise an Issue](https://github.com/executablebooks/MyST-Parser/issues/new/choose) if you think the warning is erroneous. However, in some circumstances if you wish to suppress the warning you can use the configuration option, e.g. diff --git a/docs/syntax/typography.md b/docs/syntax/typography.md index 66eb28ea..ebdf3ff5 100644 --- a/docs/syntax/typography.md +++ b/docs/syntax/typography.md @@ -295,13 +295,16 @@ that are not separated by a blank line This is not part of the footnote. ::: -````{important} -Although footnote references can be used just fine within directives, e.g.[^myref], -it is recommended that footnote definitions are not set within directives, -unless they will only be referenced within that same directive: +By default, the footnotes will be collected, sorted and moved to the end of the document, +with a transition line placed before any footnotes (that has a `footnotes` class). -This is because, they may not be available to reference in text outside that particular directive. -```` +This behaviour can be modified using the [configuration options](#sphinx/config-options): -By default, a transition line (with a `footnotes` class) will be placed before any footnotes. -This can be turned off by adding `myst_footnote_transition = False` to the config file. +```python +myst_footnote_sort = False +myst_footnote_transition = False +``` + +```{versionadded} 4.0.0 +``myst_footnote_sort`` configuration option +``` diff --git a/myst_parser/config/main.py b/myst_parser/config/main.py index b9b0c478..2b088b56 100644 --- a/myst_parser/config/main.py +++ b/myst_parser/config/main.py @@ -319,11 +319,19 @@ def __repr__(self) -> str: }, ) + footnote_sort: bool = dc.field( + default=True, + metadata={ + "validator": instance_of(bool), + "help": "Move all footnotes to the end of the document, and sort by reference order", + }, + ) + footnote_transition: bool = dc.field( default=True, metadata={ "validator": instance_of(bool), - "help": "Place a transition before any footnotes", + "help": "Place a transition before sorted footnotes", }, ) diff --git a/myst_parser/mdit_to_docutils/base.py b/myst_parser/mdit_to_docutils/base.py index 1dded859..bdd15156 100644 --- a/myst_parser/mdit_to_docutils/base.py +++ b/myst_parser/mdit_to_docutils/base.py @@ -7,7 +7,6 @@ import os import posixpath import re -from collections import OrderedDict from collections.abc import Callable, Iterable, Iterator, MutableMapping, Sequence from contextlib import contextmanager, suppress from datetime import date, datetime @@ -159,8 +158,9 @@ def sphinx_env(self) -> BuildEnvironment | None: def create_warning( self, message: str, - subtype: MystWarnings, + subtype: MystWarnings | str, *, + wtype: str | None = None, line: int | None = None, append_to: nodes.Element | None = None, ) -> nodes.system_message | None: @@ -173,6 +173,7 @@ def create_warning( self.document, message, subtype, + wtype=wtype, line=line, append_to=append_to, ) @@ -190,20 +191,6 @@ def _render_tokens(self, tokens: list[Token]) -> None: # nest tokens node_tree = SyntaxTreeNode(tokens) - - # move footnote definitions to env - self.md_env.setdefault("foot_refs", {}) - for node in node_tree.walk(include_self=True): - new_children = [] - for child in node.children: - if child.type == "footnote_reference": - label = child.meta["label"] - self.md_env["foot_refs"].setdefault(label, []).append(child) - else: - new_children.append(child) - - node.children = new_children - # render for child in node_tree.children: # skip hidden? @@ -254,6 +241,12 @@ def _render_finalise(self) -> None: self._heading_slugs ) + # ensure these settings are set for later footnote transforms + self.document.settings.myst_footnote_transition = ( + self.md_config.footnote_transition + ) + self.document.settings.myst_footnote_sort = self.md_config.footnote_sort + # log warnings for duplicate reference definitions # "duplicate_refs": [{"href": "ijk", "label": "B", "map": [4, 5], "title": ""}], for dup_ref in self.md_env.get("duplicate_refs", []): @@ -264,35 +257,6 @@ def _render_finalise(self) -> None: append_to=self.document, ) - # we don't use the foot_references stored in the env - # since references within directives/roles will have been added after - # those from the initial markdown parse - # instead we gather them from a walk of the created document - foot_refs = OrderedDict() - for refnode in findall(self.document)(nodes.footnote_reference): - if refnode["refname"] not in foot_refs: - foot_refs[refnode["refname"]] = True - - if foot_refs and self.md_config.footnote_transition: - self.current_node.append(nodes.transition(classes=["footnotes"])) - for footref in foot_refs: - foot_ref_tokens = self.md_env["foot_refs"].get(footref, []) - if len(foot_ref_tokens) > 1: - self.create_warning( - f"Multiple footnote definitions found for label: '{footref}'", - MystWarnings.MD_FOOTNOTE_DUPE, - append_to=self.current_node, - ) - - if len(foot_ref_tokens) < 1: - self.create_warning( - f"No footnote definitions found for label: '{footref}'", - MystWarnings.MD_FOOTNOTE_MISSING, - append_to=self.current_node, - ) - else: - self.render_footnote_reference(foot_ref_tokens[0]) - # Add the wordcount, generated by the ``mdit_py_plugins.wordcount_plugin``. wordcount_metadata = self.md_env.get("wordcount", {}) if wordcount_metadata: @@ -1469,11 +1433,13 @@ def render_footnote_ref(self, token: SyntaxTreeNode) -> None: refnode = nodes.footnote_reference(f"[^{target}]") self.add_line_and_source_path(refnode, token) - if not target.isdigit(): + if target.isdigit(): + # a manually numbered footnote, similar to rST ``[1]_`` + refnode += nodes.Text(target) + else: + # an auto-numbered footnote, similar to rST ``[#label]_`` refnode["auto"] = 1 self.document.note_autofootnote_ref(refnode) - else: - refnode += nodes.Text(target) refnode["refname"] = target self.document.note_footnote_ref(refnode) @@ -1481,17 +1447,36 @@ def render_footnote_ref(self, token: SyntaxTreeNode) -> None: self.current_node.append(refnode) def render_footnote_reference(self, token: SyntaxTreeNode) -> None: + """Despite the name, this is actually a footnote definition, e.g. `[^a]: ...`""" target = token.meta["label"] + if target in self.document.nameids: + # note we chose to directly omit these footnotes in the parser, + # rather than let docutils/sphinx handle them, since otherwise you end up with a confusing warning: + # WARNING: Duplicate explicit target name: "x". [docutils] + # we use [ref.footnote] as the type/subtype, rather than a myst specific warning, + # to make it more aligned with sphinx warnings for unreferenced footnotes + self.create_warning( + f"Duplicate footnote definition found for label: '{target}'", + "footnote", + wtype="ref", + line=token_line(token), + append_to=self.current_node, + ) + return + footnote = nodes.footnote() self.add_line_and_source_path(footnote, token) footnote["names"].append(target) - if not target.isdigit(): - footnote["auto"] = 1 - self.document.note_autofootnote(footnote) - else: + if target.isdigit(): + # a manually numbered footnote, similar to rST ``.. [1]`` footnote += nodes.label("", target) self.document.note_footnote(footnote) + else: + # an auto-numbered footnote, similar to rST ``.. [#label]`` + footnote["auto"] = 1 + self.document.note_autofootnote(footnote) + self.document.note_explicit_target(footnote, footnote) with self.current_node_context(footnote, append=True): self.render_children(token) diff --git a/myst_parser/mdit_to_docutils/transforms.py b/myst_parser/mdit_to_docutils/transforms.py index cd2b9d70..7815dc87 100644 --- a/myst_parser/mdit_to_docutils/transforms.py +++ b/myst_parser/mdit_to_docutils/transforms.py @@ -6,6 +6,7 @@ from docutils import nodes from docutils.transforms import Transform +from docutils.transforms.references import Footnotes from markdown_it.common.normalize_url import normalizeLink from myst_parser._compat import findall @@ -13,8 +14,132 @@ from myst_parser.warnings_ import MystWarnings, create_warning +class UnreferencedFootnotesDetector(Transform): + """Detect unreferenced footnotes and emit warnings. + + Replicates https://github.com/sphinx-doc/sphinx/pull/12730, + but also allows for use in docutils (without sphinx). + """ + + default_priority = Footnotes.default_priority + 2 + + # document: nodes.document + + def apply(self, **kwargs: t.Any) -> None: + """Apply the transform.""" + + for node in self.document.footnotes: + # note we do not warn on duplicate footnotes here + # (i.e. where the name has been moved to dupnames) + # since this is already reported by docutils + if not node["backrefs"] and node["names"]: + create_warning( + self.document, + "Footnote [{}] is not referenced.".format(node["names"][0]) + if node["names"] + else node["dupnames"][0], + wtype="ref", + subtype="footnote", + node=node, + ) + for node in self.document.symbol_footnotes: + if not node["backrefs"]: + create_warning( + self.document, + "Footnote [*] is not referenced.", + wtype="ref", + subtype="footnote", + node=node, + ) + for node in self.document.autofootnotes: + # note we do not warn on duplicate footnotes here + # (i.e. where the name has been moved to dupnames) + # since this is already reported by docutils + if not node["backrefs"] and node["names"]: + create_warning( + self.document, + "Footnote [#] is not referenced.", + wtype="ref", + subtype="footnote", + node=node, + ) + + +class SortFootnotes(Transform): + """Sort auto-numbered, labelled footnotes by the order they are referenced. + + This is run before the docutils ``Footnote`` transform, where numbered labels are assigned. + """ + + default_priority = Footnotes.default_priority - 2 + + # document: nodes.document + + def apply(self, **kwargs: t.Any) -> None: + """Apply the transform.""" + if not self.document.settings.myst_footnote_sort: + return + + ref_order: list[str] = [ + node["refname"] + for node in self.document.autofootnote_refs + if "refname" in node + ] + + def _sort_key(node: nodes.footnote) -> int: + if node["names"] and node["names"][0] in ref_order: + return ref_order.index(node["names"][0]) + return 999 + + self.document.autofootnotes.sort(key=_sort_key) + + +class CollectFootnotes(Transform): + """Transform to move footnotes to the end of the document, and sort by label.""" + + default_priority = Footnotes.default_priority + 3 + + # document: nodes.document + + def apply(self, **kwargs: t.Any) -> None: + """Apply the transform.""" + if not self.document.settings.myst_footnote_sort: + return + + footnotes: list[tuple[str, nodes.footnote]] = [] + for footnote in ( + self.document.symbol_footnotes + + self.document.footnotes + + self.document.autofootnotes + ): + label = footnote.children[0] + footnotes.append((label.astext(), footnote)) + + if ( + footnotes + and self.document.settings.myst_footnote_transition + # avoid warning: Document or section may not begin with a transition + and not all(isinstance(c, nodes.footnote) for c in self.document.children) + ): + transition = nodes.transition(classes=["footnotes"]) + transition.source = self.document.source + self.document += transition + + def _sort_key(footnote: tuple[str, nodes.footnote]) -> int | str: + label, _ = footnote + try: + # ensure e.g 10 comes after 2 + return int(label) + except ValueError: + return label + + for _, footnote in sorted(footnotes, key=_sort_key): + footnote.parent.remove(footnote) + self.document += footnote + + class ResolveAnchorIds(Transform): - """Directive for resolving `[name](#id)` type links.""" + """Transform for resolving `[name](#id)` type links.""" default_priority = 879 # this is the same as Sphinx's StandardDomain.process_doc diff --git a/myst_parser/parsers/docutils_.py b/myst_parser/parsers/docutils_.py index c16550b2..e17de44b 100644 --- a/myst_parser/parsers/docutils_.py +++ b/myst_parser/parsers/docutils_.py @@ -23,7 +23,12 @@ read_topmatter, ) from myst_parser.mdit_to_docutils.base import DocutilsRenderer -from myst_parser.mdit_to_docutils.transforms import ResolveAnchorIds +from myst_parser.mdit_to_docutils.transforms import ( + CollectFootnotes, + ResolveAnchorIds, + SortFootnotes, + UnreferencedFootnotesDetector, +) from myst_parser.parsers.mdit import create_md_parser from myst_parser.warnings_ import MystWarnings, create_warning @@ -246,7 +251,12 @@ class Parser(RstParser): translate_section_name = None def get_transforms(self): - return super().get_transforms() + [ResolveAnchorIds] + return super().get_transforms() + [ + UnreferencedFootnotesDetector, + SortFootnotes, + CollectFootnotes, + ResolveAnchorIds, + ] def parse(self, inputstring: str, document: nodes.document) -> None: """Parse source text. diff --git a/myst_parser/parsers/mdit.py b/myst_parser/parsers/mdit.py index 1c29ef10..3e65c4dd 100644 --- a/myst_parser/parsers/mdit.py +++ b/myst_parser/parsers/mdit.py @@ -61,11 +61,8 @@ def create_md_parser( .use(front_matter_plugin) .use(myst_block_plugin) .use(myst_role_plugin) - .use(footnote_plugin) + .use(footnote_plugin, inline=False, move_to_end=False, always_match_refs=True) .use(wordcount_plugin, per_minute=config.words_per_minute) - .disable("footnote_inline") - # disable this for now, because it need a new implementation in the renderer - .disable("footnote_tail") ) typographer = False diff --git a/myst_parser/parsers/sphinx_.py b/myst_parser/parsers/sphinx_.py index c8c39335..5708c950 100644 --- a/myst_parser/parsers/sphinx_.py +++ b/myst_parser/parsers/sphinx_.py @@ -14,7 +14,11 @@ read_topmatter, ) from myst_parser.mdit_to_docutils.sphinx_ import SphinxRenderer -from myst_parser.mdit_to_docutils.transforms import ResolveAnchorIds +from myst_parser.mdit_to_docutils.transforms import ( + CollectFootnotes, + ResolveAnchorIds, + SortFootnotes, +) from myst_parser.parsers.mdit import create_md_parser from myst_parser.warnings_ import create_warning @@ -46,7 +50,11 @@ class MystParser(SphinxParser): translate_section_name = None def get_transforms(self): - return super().get_transforms() + [ResolveAnchorIds] + return super().get_transforms() + [ + SortFootnotes, + CollectFootnotes, + ResolveAnchorIds, + ] def parse(self, inputstring: str, document: nodes.document) -> None: """Parse source text. diff --git a/myst_parser/sphinx_ext/main.py b/myst_parser/sphinx_ext/main.py index a72527c4..eda129ac 100644 --- a/myst_parser/sphinx_ext/main.py +++ b/myst_parser/sphinx_ext/main.py @@ -5,7 +5,11 @@ import sphinx from docutils import nodes from sphinx.application import Sphinx +from sphinx.transforms import ( + UnreferencedFootnotesDetector as SphinxUnreferencedFootnotesDetector, +) +from myst_parser.mdit_to_docutils.transforms import UnreferencedFootnotesDetector from myst_parser.parsers.docutils_ import ( depart_container_html, depart_rubric_html, @@ -39,6 +43,12 @@ def setup_sphinx(app: Sphinx, load_parser: bool = False) -> None: app.add_role("sub-ref", SubstitutionReferenceRole()) app.add_directive("figure-md", FigureMarkdown) + # TODO currently we globally replace sphinx's transform, + # to overcome issues it has (https://github.com/sphinx-doc/sphinx/pull/12730), + # but once this PR is merged/released, we should remove this + app.registry.transforms.remove(SphinxUnreferencedFootnotesDetector) + app.add_transform(UnreferencedFootnotesDetector) + app.add_post_transform(MystReferenceResolver) # override only the html writer visit methods for rubric, to use the "level" attribute diff --git a/myst_parser/warnings_.py b/myst_parser/warnings_.py index 14183544..8a90280f 100644 --- a/myst_parser/warnings_.py +++ b/myst_parser/warnings_.py @@ -5,7 +5,7 @@ from collections.abc import Sequence from enum import Enum -from docutils import nodes +from docutils import nodes, utils class MystWarnings(Enum): @@ -23,10 +23,6 @@ class MystWarnings(Enum): """Issue reading front-matter.""" MD_DEF_DUPE = "duplicate_def" """Duplicate Markdown reference definition.""" - MD_FOOTNOTE_DUPE = "footnote" - """Duplicate Markdown footnote definition.""" - MD_FOOTNOTE_MISSING = "footnote" # noqa: PIE796 - """Missing Markdown footnote definition.""" MD_HEADING_NON_CONSECUTIVE = "header" """Non-consecutive heading levels.""" @@ -98,8 +94,10 @@ def _is_suppressed_warning( def create_warning( document: nodes.document, message: str, - subtype: MystWarnings, + subtype: MystWarnings | str, *, + wtype: str | None = None, + node: nodes.Element | None = None, line: int | None = None, append_to: nodes.Element | None = None, ) -> nodes.system_message | None: @@ -114,8 +112,10 @@ def create_warning( # Note also that in general we want to show the type/subtype in the warning message, # but this was added as an option to sphinx in v7.3, and made the default in v8.0. - wtype = "myst" - message_with_type = f"{message} [{wtype}.{subtype.value}]" + type_str = wtype if wtype is not None else "myst" + subtype_str = subtype if isinstance(subtype, str) else subtype.value + + message_with_type = f"{message} [{type_str}.{subtype_str}]" if hasattr(document.settings, "env"): # Sphinx @@ -124,24 +124,31 @@ def create_warning( logger = getLogger(__name__) logger.warning( message, - type=wtype, - subtype=subtype.value, - location=(document["source"], line), + type=type_str, + subtype=subtype_str, + location=node if node is not None else (document["source"], line), ) if _is_suppressed_warning( - wtype, subtype.value, document.settings.env.config.suppress_warnings + type_str, subtype_str, document.settings.env.config.suppress_warnings ): return None - msg_node = _create_warning_node(message_with_type, document["source"], line) + if node is not None: + _source, _line = utils.get_source_line(node) + else: + _source, _line = document["source"], line + msg_node = _create_warning_node(message_with_type, _source, _line) else: # docutils if _is_suppressed_warning( - wtype, subtype.value, document.settings.myst_suppress_warnings or [] + type_str, subtype_str, document.settings.myst_suppress_warnings or [] ): return None - msg_node = document.reporter.warning( - message_with_type, **({"line": line} if line is not None else {}) - ) + kwargs = {} + if node is not None: + kwargs["base_node"] = node + elif line is not None: + kwargs["line"] = line + msg_node = document.reporter.warning(message_with_type, **kwargs) if append_to is not None: append_to.append(msg_node) diff --git a/pyproject.toml b/pyproject.toml index 8933f90b..4ea84ba9 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -36,7 +36,7 @@ dependencies = [ "docutils>=0.19,<0.22", "jinja2", # required for substitutions, but let sphinx choose version "markdown-it-py~=3.0", - "mdit-py-plugins~=0.4", + "mdit-py-plugins~=0.4,>=0.4.1", "pyyaml", "sphinx>=7,<9", ] diff --git a/tests/test_renderers/fixtures/docutil_syntax_elements.md b/tests/test_renderers/fixtures/docutil_syntax_elements.md index 6badaa4c..67668bf3 100644 --- a/tests/test_renderers/fixtures/docutil_syntax_elements.md +++ b/tests/test_renderers/fixtures/docutil_syntax_elements.md @@ -430,7 +430,7 @@ Link Definition in nested directives: . -Footnotes: +Footnotes [APPLY TRANSFORMS]: . [^a] @@ -438,16 +438,19 @@ Footnotes: . - + + 1 - + +