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 — Infra: Improve email and link processing and rendering in headers by CAM-Gerlach · Pull Request #2467 · python/peps

Conversation

@CAM-Gerlach

@CAM-Gerlach CAM-Gerlach commented Mar 25, 2022

Copy link
Copy Markdown
Member

After some time reading, writing and interacting with the recently-added automatic formatting of links in the Discussions-To, Post-History and Resolution headers originally added in #2351 and improved in #2361, I've implemented a number of improvements and fixes that make both the URL and the link text more useful, handle many additional common cases, include the Post-History field as well, and prepare for other potential future enhancements. In particular, they now:

  • Link to the relevant main page of python.org and googlegroups.com mailing lists for non-thread links, where users can subscribe to the list, browse relevant threads and post a reply (via mailto or web), as well as view more information about the venue
  • Support Discourse, with the link title "Discourse thread" for threads and " Discourse category" (e.g. "Packaging Discourse category") for categories, rather than a bare link
  • Explicitly describe the target type in the link text (e.g. "Python-Ideas list", "Typing-SIG thread", "Python-Dev message", "Discord post", "Packaging Discourse category", etc), making it more clear and less confusing at a glance what they represent
  • Show the friendly name/description of the link target in the titletext when hovering over links in the Post-History section, making it easier to see where and how the PEP was posted to each time without having to click through to the link
  • Work for list info pages for Mailman and Mailman3, not just the list archives
  • Are refactored to be more cleanly implemented and easily extensible to future link types and the logic re-used for other purposes

This also makes it easy to adopt the Post-History field formatting for the Resolution field, in order to use the much more useful and interesting resolution date as the link text, while retaining the auto-generated venue/target information as the title text, though that will be a separate followup issue/PR. I will have another PR shortly (already almost done) that updates the linters to provide more immediate, specific and precise feedback on the header field format to complement the various recent backend improvements in automatically parsing and rendering them.

Sorry, something went wrong.

Uh oh!

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

All reactions
@CAM-Gerlach CAM-Gerlach added the infra Core infrastructure for building and rendering PEPs label Mar 25, 2022
@CAM-Gerlach
CAM-Gerlach requested review from JelleZijlstra and hugovk March 25, 2022 04:15
@CAM-Gerlach CAM-Gerlach self-assigned this Mar 25, 2022
@CAM-Gerlach
CAM-Gerlach requested review from AA-Turner and warsaw as code owners March 25, 2022 04:15
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 25, 2022
@CAM-Gerlach CAM-Gerlach mentioned this pull request Mar 25, 2022
PEP 630: Format and copyedit prior to conversion into a docs HOWTO guide #2459
Merged
JelleZijlstra
JelleZijlstra reviewed Mar 26, 2022
View reviewed changes

@JelleZijlstra JelleZijlstra 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.

Looks good, just one nit

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread pep_sphinx_extensions/pep_processor/transforms/pep_headers.py Resolved

Uh oh!

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

hugovk
hugovk reviewed Mar 27, 2022
View reviewed changes

@hugovk hugovk 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.

It would be nice to add unit tests for some of these, especially the text processing functions not doing Sphinx things. Not a blocker for this PR though!

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread pep-0009.txt
Content-Type: text/x-rst
Created: 14-Aug-2001
Post-History:
Resolution: https://mail.python.org/mailman/private/peps/2016-January/001165.html

@hugovk hugovk Mar 25, 2022

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.

For this link I get:

Private Archive Error - No such list peps

Does this link work for others?

Perhaps this later 2016 link withdrawing the PEP would be a good replacement?

https://mail.python.org/archives/list/[email protected]/thread/2YMHVPRDWGQLA5A2FKXE2JMLM2HQEEGW/


(As an aside, I note the reason for not renaming .txt to .rst was tooling rather than Git churn, and PEP editors were open to someone doing it)

The PEP
editors will not be converting the legacy PEPs to reST, nor will we currently
be renaming the relevant PEP source files to end with ".rst" since there's too
much tooling that would have to change to do so. However, if either task
really interests you, please get in touch with the PEP editors.

Sorry, something went wrong.

Uh oh!

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

All reactions

@CAM-Gerlach CAM-Gerlach Mar 31, 2022

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.

I got the same result as you, and coupled with it breaking the parsing which would require special case workarounds, so I'd removed it, I went ahead and replaced it with the much better link you suggested, both here and in #2484 (which I suggest approving and merging first once it is ready, and dropping the change here).

Sorry, something went wrong.

Uh oh!

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

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@CAM-Gerlach CAM-Gerlach mentioned this pull request Mar 30, 2022
Lint: Update headers and checks per current guidance & provide helpful feedback #2484
Merged
@CAM-Gerlach
CAM-Gerlach force-pushed the infra-clarify-discussions-to-target-type branch from 92994be to e4f3c01 Compare March 31, 2022 06:56
@CAM-Gerlach

CAM-Gerlach commented Mar 31, 2022

Copy link
Copy Markdown
Member Author

NB: #2484 should preferably be approved and merged before this PEP, since the former ensures all headers will be parsed using the processing here, instead of silently falling back to a raw link, avoids any edge cases and contains one conflicting line, that makes more sense there and will be easier to drop here.

All reactions

Sorry, something went wrong.

Uh oh!

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

@CAM-Gerlach

CAM-Gerlach commented Mar 31, 2022

Copy link
Copy Markdown
Member Author

It would be nice to add unit tests for some of these, especially the text processing functions not doing Sphinx things. Not a blocker for this PR though!

Yeah, I definitely thought about doing that, and wrote and tested a bunch of informal test cases, but ultimately decided since its a fairly non-trivial undertaking and likely should be done elsewhere, decided to defer that to a potential future PR.

hugovk reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

hugovk
hugovk approved these changes Mar 31, 2022
View reviewed changes
raghavthind2005
raghavthind2005 approved these changes Apr 1, 2022
View reviewed changes
@CAM-Gerlach

CAM-Gerlach commented Apr 15, 2022

Copy link
Copy Markdown
Member Author

Updating to retrigger the CLA signing bot

All reactions

Sorry, something went wrong.

Uh oh!

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

@CAM-Gerlach
CAM-Gerlach force-pushed the infra-clarify-discussions-to-target-type branch from e4f3c01 to 7ff9c3a Compare April 15, 2022 21:28
@CAM-Gerlach CAM-Gerlach mentioned this pull request Apr 16, 2022
Waste less vertical space at top of rendered PEP #2514
Closed
CAM-Gerlach added 7 commits April 17, 2022 16:31
@CAM-Gerlach
Infra: Include mailing list link type in text for clarity, & refactor
5ee6938
@CAM-Gerlach
Infra: Support mailing list info pages as well as archives
f9d14d3
@CAM-Gerlach
Infra: Pretty-format Discourse categories/threads in headers as well
77debe1
@CAM-Gerlach
Infra: Show friendly-name of target in Post-History link hover text
a2d50c4
@CAM-Gerlach
Infra: Automatically link list addresses to list pages in Discussions-To
e10e775
@CAM-Gerlach
Infra: Use simpler check for title in Discourse URL header processing
ebed4ce
@CAM-Gerlach
Infra: Automatically trim trailing commas & whitespace in header values
a0bc50f
@CAM-Gerlach
CAM-Gerlach force-pushed the infra-clarify-discussions-to-target-type branch from 7ff9c3a to a0bc50f Compare April 17, 2022 22:21
@CAM-Gerlach

CAM-Gerlach commented Apr 17, 2022

Copy link
Copy Markdown
Member Author

Thanks to @hugovk 's observation on #2531 , I'd forgotten that the build system wasn't actually trimming trailing commas in the header values, meaning that while the linting checks in #2484 allow them, they are actually displayed in the output, which is undesired. With this change, the header transform now automatically elides trailing spaces and commas in header fields, so authors are free to use them in their source files if they choose without them effecting the output. I've tested them with #2531 and some other examples in different fields and it all works great.

hugovk reacted with thumbs up 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 Apr 17, 2022
PEP 639: Update header, footer, link, reference and code block syntax #2531
Merged
@hugovk
hugovk merged commit efeae28 into python:main Apr 19, 2022
@hugovk

hugovk commented Apr 19, 2022

Copy link
Copy Markdown
Member

Thank you!

All reactions

Sorry, something went wrong.

Uh oh!

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

@CAM-Gerlach

CAM-Gerlach commented Apr 19, 2022

Copy link
Copy Markdown
Member Author

As mentioned a number of times in a number of places, #2484 was soft-blocking this one (and #2531 blocking it in turn) due to an expected merge conflicts and to avoid any inconsistencies with the display of pre-normalized links/headers. However, it turns out that I managed to avoid a merge conflict after all (by ensuring identical changes on both PRs), while the anonymization issue came up on #2484 , so it didn't end up being that bad to merge this first after all.

hugovk reacted with thumbs up 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 Apr 20, 2022
PEP 11: fix nit typo of my name #2538
Merged
@hugovk hugovk mentioned this pull request Apr 20, 2022
Infra: add tests #2545
Merged
@CAM-Gerlach CAM-Gerlach mentioned this pull request Apr 21, 2022
PEP 11: Add Discussions section #2544
Merged
@CAM-Gerlach CAM-Gerlach mentioned this pull request May 5, 2022
PEP 690: Added Post-History #2575
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

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments
@hugovk hugovk hugovk approved these changes
@warsaw warsaw Awaiting requested review from warsaw
@AA-Turner AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner
+1 more reviewer
@raghavthind2005 raghavthind2005 raghavthind2005 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

@CAM-Gerlach CAM-Gerlach

Labels

CLA signed infra Core infrastructure for building and rendering PEPs

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 @hugovk @JelleZijlstra @raghavthind2005 @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 01402520 — 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.