GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Don't auto-add inline links to ref section & rm if empty, per #2130 by CAM-Gerlach · Pull Request #2155 · python/peps

Conversation

@CAM-Gerlach

@CAM-Gerlach CAM-Gerlach commented Nov 19, 2021

Copy link
Copy Markdown
Member

First step to implementing #2130 , as agreed with @gvanrossum and the PEP editor team.

When building, don't add redundant footnotes and references entries for URLs that are already directly linked inline. This avoids an unnecessary, potentially confusing footnote for each link, and taking up additional space in the references section for no real benefit, plus simplifies the build code and should improve build time, especially for Sphinx. Furthermore, if the references section is empty (not including now-invisible link targets), remove it, as we did before (but in a more robust manner). This allows past and future PEPs to still use inline references with targets stored in the references section, while avoiding showing a now-empty references section.

These are both implemented for Sphinx and the legacy builder, and I visually inspected a variety of PEPs with various cases with both builders to ensure the desired results were achieved, and there were no obvious side effects from this change.

Following merging this PR, following the plan outlined in #2130 , I'll proceed with one updating the meta-PEP docs in PEP 0, PEP 1 and PEP 12 to reflect the revised policy of using standard reST links (inline or with separate targets) rather than the references section, and follow that with a PR updating the relative handful of references in the other active meta-PEPs, for consistency.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@CAM-Gerlach
Don't auto-add inline links to ref section & rm if empty, per python#…
129ecb2
…2130
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 19, 2021
@CAM-Gerlach CAM-Gerlach mentioned this pull request Nov 19, 2021
Discourage using References? #2130
Closed
@brettcannon
brettcannon requested a review from a team November 19, 2021 22:51
gvanrossum
gvanrossum approved these changes Nov 20, 2021
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I trust you.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

CAM-Gerlach reacted with heart emoji
All reactions
  • ❤️ 1 reaction
AA-Turner
AA-Turner reviewed Nov 24, 2021
View reviewed changes
Comment thread pep_sphinx_extensions/pep_processor/transforms/pep_footer.py Outdated
Comment on lines 36 to 42
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

@AA-Turner AA-Turner Nov 24, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@AA-Turner AA-Turner Nov 24, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

CAM-Gerlach reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@CAM-Gerlach CAM-Gerlach Nov 24, 2021

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AA-Turner reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@CAM-Gerlach @AA-Turner
Further simplify footer references section cleanup code
5788d8a
Co-Authored-By: Adam Turner <[email protected]>
@CAM-Gerlach CAM-Gerlach mentioned this pull request Dec 18, 2021
PEP 639: Make requested additions and changes, add additional helpful info, reformat, update and copyedit #2164
Merged
@CAM-Gerlach

CAM-Gerlach commented Dec 18, 2021

Copy link
Copy Markdown
Member Author

Hey @JelleZijlstra or others, are there any blockers to merging this? This would make my PEP 639 over five full pages shorter (since it has around 100 links) and it seems we all agree on it, and then I can proceed with Part 2, a PR updating PEP 1 and PEP 12 , etc., to reflect this.

This also unblocks/avoids merge conflicting with @AA-Turner 's important work to implement PEP 676, which among other things actually opens up a much better approach to addressing your concerns about PEP 639's excessive length, drastically reducing it (on top of cutting down verbosity and specific portions that can be elided, which I'm working on with @pradyunsg ) by moving around two thirds of the PEP text (which is appendices, documentation of rejected ideas and ancillary material) to separate supporting files in the pep-0639 subdirectory, without loosing it completely.

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra
JelleZijlstra merged commit f82973d into python:main Dec 18, 2021
@AA-Turner

AA-Turner commented Dec 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

& gone in https://python.github.io/peps/pep-0639!

CAM-Gerlach and hugovk reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach

CAM-Gerlach commented Dec 19, 2021

Copy link
Copy Markdown
Member Author

Thanks so much @JelleZijlstra , @gvanrossum and @AA-Turner !

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Dec 19, 2021

Copy link
Copy Markdown
Member

Thank you! Have you been promoted to PEP editor yet?

CAM-Gerlach and hugovk reacted with laugh emoji
All reactions
  • 😄 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach

CAM-Gerlach commented Dec 19, 2021

Copy link
Copy Markdown
Member Author

Hahaha @gvanrossum , I'm still at "PEP intern" status 😆 But you never know, maybe someday, given that I sure am a lot better at nitpicking proofreading, rewriting copyediting, and pedantry formatting English technical documents than I am at programming, even in my favorite British-comedy-troupe-themed language created by some Dutch guy...

pradyunsg and hugovk reacted with laugh emoji
All reactions
  • 😄 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach

CAM-Gerlach commented Jan 12, 2022

Copy link
Copy Markdown
Member Author

FYI @AA-Turner , looks like our 5788d8a resulted in my multiple-layers-of-indirection links in PEP 639 resolving to anchors on the page rather than the links themselves with the legacy build system (things work fine with your new build system, and my previous commit also works). Not sure how I missed this in testing, sorry...I must not have carefully checked a range of links on that specific PR on the old build system for the final version of that commit, despite having done so for the previous one. Seems your trust was misplaced after all, @gvanrossum 😢

I suspect it has to do with the delayed callback to delete the references section being necessary so that the links get resolved before the section is deleted and their original targets removed. I'll test and confirm and have a followup PR shortly to fix that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach CAM-Gerlach mentioned this pull request Jan 12, 2022
Set low priority for rm-ing refs section so refs still resolve correctly #2227
Merged
@gvanrossum

gvanrossum commented Jan 12, 2022

Copy link
Copy Markdown
Member

Don’t worry CAM, I’ve done worse. :-)

CAM-Gerlach reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach CAM-Gerlach mentioned this pull request Jan 12, 2022
Fix refs section removal breaking external links on Sphinx build too #2229
Merged
@CAM-Gerlach
CAM-Gerlach deleted the no-autoadd-inline-refs branch January 12, 2022 19:13
@CAM-Gerlach

CAM-Gerlach commented Jan 12, 2022

Copy link
Copy Markdown
Member Author

Yeah...after all, you were the one responsible for adding math.tau to the standard library 😆

AA-Turner and pradyunsg reacted with laugh emoji
All reactions
  • 😄 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@CAM-Gerlach CAM-Gerlach mentioned this pull request Feb 4, 2022
PEP 12: Recommend and explain native RST hyperlinks (versus footnotes) #2302
Merged
@erlend-aasland erlend-aasland mentioned this pull request May 13, 2022
pep8/greppable exception messages erlend-aasland/peps#1
Closed
@erlend-aasland erlend-aasland mentioned this pull request Jun 27, 2022
pep 687/mark as accepted erlend-aasland/peps#2
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AA-Turner AA-Turner AA-Turner left review comments
@gvanrossum gvanrossum gvanrossum approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

CLA signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@CAM-Gerlach @AA-Turner @gvanrossum @JelleZijlstra @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.