Skip to content
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

Don't auto-add inline links to ref section & rm if empty, per #2130 #2155

Merged
merged 2 commits into from
Dec 18, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions pep2html.py
Original file line number Diff line number Diff line change
@@ -45,14 +45,15 @@
import random
import time
from io import open
from pathlib import Path
try:
from html import escape
except ImportError:
from cgi import escape

from docutils import core, nodes, utils
from docutils.readers import standalone
from docutils.transforms import peps, frontmatter, Transform
from docutils.transforms import frontmatter, misc, peps, Transform
from docutils.parsers import rst

class DataError(Exception):
@@ -433,6 +434,46 @@ def apply(self):
elif name == 'version' and len(body):
utils.clean_rcs_keywords(para, self.rcs_keyword_substitutions)


class PEPFooter(Transform):
"""Remove the References section if it is empty when rendered."""

# Uses same priority as docutils.transforms.TargetNotes
default_priority = 520

def apply(self):
pep_source_path = Path(self.document["source"])
if not pep_source_path.match("pep-*"):
return # not a PEP file, exit early

doc = self.document
reference_section = None

# Iterate through sections from the end of the document
for i, section in enumerate(reversed(doc)):
if not isinstance(section, nodes.section):
continue
title_words = section[0].astext().lower().split()
if "references" in title_words:
reference_section = section
break

# Remove references section if there are no displayed footnotes
if reference_section:
pending = nodes.pending(
misc.CallBack, details={"callback": _cleanup_callback})
reference_section.append(pending)
self.document.note_pending(pending, priority=1)


def _cleanup_callback(pending):
"""Remove an empty "References" section."""
for footer_node in pending.parent:
if isinstance(footer_node, (nodes.title, nodes.target, nodes.pending)):
return
pending.parent.parent.remove(pending.parent)


class PEPReader(standalone.Reader):

supported = ('pep',)
@@ -453,7 +494,7 @@ def get_transforms(self):
transforms.remove(frontmatter.DocTitle)
transforms.remove(frontmatter.SectionSubTitle)
transforms.remove(frontmatter.DocInfo)
transforms.extend([PEPHeaders, peps.Contents, peps.TargetNotes])
transforms.extend([PEPHeaders, peps.Contents, PEPFooter])
return transforms

settings_default_overrides = {'pep_references': 1, 'rfc_references': 1}
56 changes: 15 additions & 41 deletions pep_sphinx_extensions/pep_processor/transforms/pep_footer.py
Original file line number Diff line number Diff line change
@@ -5,22 +5,16 @@
from docutils import nodes
from docutils import transforms
from docutils.transforms import misc
from docutils.transforms import references

from pep_sphinx_extensions import config


class PEPFooter(transforms.Transform):
"""Footer transforms for PEPs.
- Appends external links to footnotes.
- Removes the References section if it is empty when rendered.
- Creates a link to the (GitHub) source text.
TargetNotes:
Locate the `References` section, insert a placeholder at the end
for an external target footnote insertion transform, and schedule
the transform to run immediately.
Source Link:
Create the link to the source file from the document source path,
and append the text to the end of the document.
@@ -36,42 +30,22 @@ def apply(self) -> None:
return # not a PEP file, exit early

doc = self.document[0]
reference_section = copyright_section = None
reference_section = None

# Iterate through sections from the end of the document
num_sections = len(doc)
for i, section in enumerate(reversed(doc)):
if not isinstance(section, nodes.section):
continue
title_words = section[0].astext().lower().split()
if "references" in title_words:
reference_section = section
break
Copy link
Member

@AA-Turner AA-Turner Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for i, section in enumerate(reversed(doc)):
if not isinstance(section, nodes.section):
continue
title_words = section[0].astext().lower().split()
if "references" in title_words:
reference_section = section
break
for i, section in enumerate(reversed(self.document[0])):
if not isinstance(section, nodes.section):
continue
title_words = section[0].astext().lower().split()
if "references" in title_words:
# Remove references section if there are no displayed
# footnotes (it only has title & link target nodes)
if all(isinstance(ref_node, (nodes.title, nodes.target)) for ref_node in section):
section.parent.remove(section)
break

Only commenting on the Sphinx version but I think this can be far simpler -- use the suggested in the loop itself, and then remove the if reference_section stanza and the entire _cleanup_callback -- as the pending was only needed due to the TargetNotes transform AFAIK.

Tested (briefly) locally and identical output versus your patch.

A

Copy link
Member

@AA-Turner AA-Turner Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to add the full diff in the collapsible bit but it wasn't having it -- hence long text below.
A

diff --git a/pep_sphinx_extensions/pep_processor/transforms/pep_footer.py b/pep_sphinx_extensions/pep_processor/transforms/pep_footer.py
index 5fa1b3844..966a6aacb 100644
--- a/pep_sphinx_extensions/pep_processor/transforms/pep_footer.py
+++ b/pep_sphinx_extensions/pep_processor/transforms/pep_footer.py
@@ -4,8 +4,6 @@ import subprocess
 
 from docutils import nodes
 from docutils import transforms
-from docutils.transforms import misc
-from docutils.transforms import references
 
 from pep_sphinx_extensions import config
 
@@ -13,14 +11,9 @@ from pep_sphinx_extensions import config
 class PEPFooter(transforms.Transform):
     """Footer transforms for PEPs.
 
-     - Appends external links to footnotes.
+     - Removes the References section if it is empty when rendered.
      - Creates a link to the (GitHub) source text.
 
-    TargetNotes:
-        Locate the `References` section, insert a placeholder at the end
-        for an external target footnote insertion transform, and schedule
-        the transform to run immediately.
-
     Source Link:
         Create the link to the source file from the document source path,
         and append the text to the end of the document.
@@ -35,43 +28,17 @@ class PEPFooter(transforms.Transform):
         if not pep_source_path.match("pep-*"):
             return  # not a PEP file, exit early
 
-        doc = self.document[0]
-        reference_section = copyright_section = None
-
         # Iterate through sections from the end of the document
-        num_sections = len(doc)
-        for i, section in enumerate(reversed(doc)):
+        for i, section in enumerate(reversed(self.document[0])):
             if not isinstance(section, nodes.section):
                 continue
             title_words = section[0].astext().lower().split()
             if "references" in title_words:
-                reference_section = section
+                # Remove references section if there are no displayed
+                # footnotes (it only has title & link target nodes)
+                if all(isinstance(ref_node, (nodes.title, nodes.target)) for ref_node in section):
+                    section.parent.remove(section)
                 break
-            elif "copyright" in title_words:
-                copyright_section = num_sections - i - 1
-
-        # Add a references section if we didn't find one
-        if not reference_section:
-            reference_section = nodes.section()
-            reference_section += nodes.title("", "References")
-            self.document.set_id(reference_section)
-            if copyright_section:
-                # Put the new "References" section before "Copyright":
-                doc.insert(copyright_section, reference_section)
-            else:
-                # Put the new "References" section at end of doc:
-                doc.append(reference_section)
-
-        # Add and schedule execution of the TargetNotes transform
-        pending = nodes.pending(references.TargetNotes)
-        reference_section.append(pending)
-        self.document.note_pending(pending, priority=0)
-
-        # If there are no references after TargetNotes has finished, remove the
-        # references section
-        pending = nodes.pending(misc.CallBack, details={"callback": _cleanup_callback})
-        reference_section.append(pending)
-        self.document.note_pending(pending, priority=1)
 
         # Add link to source text and last modified date
         if pep_source_path.stem != "pep-0000":
@@ -79,16 +46,6 @@ class PEPFooter(transforms.Transform):
             self.document += _add_commit_history_info(pep_source_path)
 
 
-def _cleanup_callback(pending: nodes.pending) -> None:
-    """Remove an empty "References" section.
-
-    Called after the `references.TargetNotes` transform is complete.
-
-    """
-    if len(pending.parent) == 2:  # <title> and <pending>
-        pending.parent.parent.remove(pending.parent)
-
-
 def _add_source_link(pep_source_path: Path) -> nodes.paragraph:
     """Add link to source text on VCS (GitHub)"""
     source_link = config.pep_vcs_url + pep_source_path.name

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, great catch—I wasn't 100% sure if I could drop the deferred callback. I applied the above manually to both the Sphinx and legacy build systems, and also dropped the no longer used enumerate and index variables and fixed the inconsistent quote chars. I tested it myself locally on both systems and a handful of different PEPs (some with different numbers of hardcoded and link references, and some with a condition of both) and everything rendered identical to before.

elif "copyright" in title_words:
copyright_section = num_sections - i - 1

# Add a references section if we didn't find one
if not reference_section:
reference_section = nodes.section()
reference_section += nodes.title("", "References")
self.document.set_id(reference_section)
if copyright_section:
# Put the new "References" section before "Copyright":
doc.insert(copyright_section, reference_section)
else:
# Put the new "References" section at end of doc:
doc.append(reference_section)

# Add and schedule execution of the TargetNotes transform
pending = nodes.pending(references.TargetNotes)
reference_section.append(pending)
self.document.note_pending(pending, priority=0)

# If there are no references after TargetNotes has finished, remove the
# references section
pending = nodes.pending(misc.CallBack, details={"callback": _cleanup_callback})
reference_section.append(pending)
self.document.note_pending(pending, priority=1)

# Remove references section if there are no displayed footnotes
if reference_section:
pending = nodes.pending(misc.CallBack, details={"callback": _cleanup_callback})
reference_section.append(pending)
self.document.note_pending(pending, priority=1)

# Add link to source text and last modified date
if pep_source_path.stem != "pep-0000":
@@ -80,13 +54,13 @@ def apply(self) -> None:


def _cleanup_callback(pending: nodes.pending) -> None:
"""Remove an empty "References" section.
Called after the `references.TargetNotes` transform is complete.
"""
if len(pending.parent) == 2: # <title> and <pending>
pending.parent.parent.remove(pending.parent)
"""Remove an empty "References" section."""
for ref_node in pending.parent:
# Don't remove section if has more than title, link targets and pending
if not isinstance(
ref_node, (nodes.title, nodes.target, nodes.pending)):
return
pending.parent.parent.remove(pending.parent)


def _add_source_link(pep_source_path: Path) -> nodes.paragraph: