-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Infra: Improve email and link processing and rendering in headers #2467
Infra: Improve email and link processing and rendering in headers #2467
Conversation
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
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!
@@ -8,7 +8,8 @@ Type: Process | |||
Content-Type: text/x-rst | |||
Created: 14-Aug-2001 | |||
Post-History: | |||
Resolution: https://mail.python.org/mailman/private/peps/2016-January/001165.html |
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/python-dev@python.org/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.
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).
92994be
to
e4f3c01
Compare
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. |
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. |
Updating to retrigger the CLA signing bot |
e4f3c01
to
7ff9c3a
Compare
7ff9c3a
to
a0bc50f
Compare
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. |
Thank you! |
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. |
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:
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.