Several PEPs: Use explicit :pep: and :rfc: roles
#2209
Conversation
43910e0
to
fb18a0c
Compare
|
I think the best way forwards for this PR is likely to split it up. @CAM-Gerlach A |
| @@ -777,23 +777,11 @@ References and Footnotes | |||
| for retrieving older revisions, and can also be browsed via HTTP here: | |||
| https://github.com/python/peps | |||
|
|
|||
| .. [2] PEP 2, Procedure for Adding New Modules | |||
| (http://www.python.org/dev/peps/pep-0002) | |||
There was a problem hiding this comment.
The previous system was a bit weird with its footnotes instead of inline links, but one good point was that you could see the linked PEP title without following the link.
Since we are in the PEP repo (with the data on hand without network requests needed), could the code be adapted so that the PEP number and title is used as HTML title attribute on the generated links?
(Maybe not if it’s Sphinx handling it and not any repo-specific code…)
Edit: I found PEPRole thanks to the nice new build process doc!
There was a problem hiding this comment.
Fantastic suggestion! That would be really nice and something I've been wanting for a while myself. (To note, with manual footnotes you still have to click the link to go to the footnote to see it of course, it just doesn't generate a HTTP request)
There was a problem hiding this comment.
Will have a look, seems pretty easy.
I trust your judgement, but my concern is that merge conflicts are going to accumulate (with this PR, and with others once its merged) the longer the changes to all the PEPs doesn't get merged. I've been holding off on going ahead with further changes to PEP 639 until that happens given the high number of merge conflicts that would result either way (dozens of lines that mention other PEPs, especially PEP 621) and I'd really like not to delay it further (and the bigger changes, moving much of the ancillary PEP content to separate documents, is blocked on PEP 676 adoption/implementation unless I hack a way to do it into the legacy build script). Also, re-opening it again as another PR is going to ping all the codeowners again and create double the churn on that front, after we already bit the bullet this time. So I'd advise still using this as the PR to do the mass-changes, if you're going to split it up, and minimize further changes here that ping everybody.
I saw that; makes sense to me to allow it. |
fb18a0c
to
fe00d69
Compare
Done |
True. There are 3 somewhat distinct changes here:
I could split out the last two, but it would still ping people. (I could try deleting CODEOWNERS on the PR and then dropping that commit). I went through all these manually a few times, so I'm confident in the changeset -- perhaps you could spot-check a few files, and if all looks good we merge and handle (hopefully potential) clean up later? A |
|
Sounds like a good plan, I'm +1 on this PR (as previously expressed, I share others' concerns about churn and pinging too many people for these types of broad changes, and particularly merge conflicts, but much of that cost is already spent, its a one-time thing and there's a clear rationale behind it with substantial benefits, so in this case I'm all for going ahead with it promptly). I've previously looked over a lot of the deltas and thought you did a thoughtful, commendable job handling the variety of cases that were present, at least for the fraction I looked over. I'll render it locally and examine a variety of cases further, and then
I don't think that would work; per the docs the |
Thanks again for all your hard, diligent work on this, @AA-Turner .
I read a substantial fraction of the source code changes, and spot-checked a variety of the changes in a number of PEPs rendered via both the legacy and the new build system, and everything looks very good to me! While somewhat noisy, I can understand the reasoning behind it, and in many cases the changes were not just mechanical, but also quite thoughtful and made it easier, more direct, more robust and more specific to reference the items discussed, while reducing verbosity and unnecessary indirection. Furthermore, these changes allow users much greater power and flexibility when linking PEPs, RFCs and more, which already came in handy in at least one very recent PEP. I'm in favor of merging this PEP as it stands, so we can iterate further (as in addition to improvements to the build system, this is blocking further changes on PEP 639, PEP 1 and others).
It would be really nice to have the PEP name (including the number, and ideally the section too) be the titletext when hovering over links, but that can be a separate PR.
FYI, rst is used instead of rfc in the commit messages; I suggest whoever squash-merges it fix those typos in the commit summary.
fe00d69
to
5e7e3f5
Compare
:pep: and :rst: roles
:pep: and :rst: roles:pep: and :rst: roles
:pep: and :rst: roles:pep: and :rfc: roles
|
Thanks @CAM-Gerlach; will tackle link title text in a follow-up change A |
|
Thanks @AA-Turner ! |
This is a more optional companion to #2208 -- please review that one first!
This PR seeks to disable the Docutils inliner for PEP and RFC references. The inliner looks for patterns of
pep-\d+(.txt)?orPEP\s+\d+for PEPs, andRFC(-|\s+)?\d+for RFCs. It requires mokey-patching in the implementation and picks up text that isn't intended to be an explicit reference, for example mailing list thread titles, or branch names with apep-nnnpattern.To do this in the most backwards-compatible requires changing all PEP nnnn text to
:pep:`nnnn`(similar for RFCs). The:pep:role is supported in plain Docutils, and is currently used in several PEPs, so will work with the currentpep2htmlbased system during the transition period.I did the replacements over a few days manually going through every PEP. I'm aware though that this touches a large number of files (395), so may be challenging to review.
If it is easier I could break this PR up, or just withdraw it entirely -- please let me know either way.
A