Several PEPs: Use explicit :pep: and :rfc: roles
#2209
Conversation
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 ✔️ from my end assuming I don't find any particular issues. Thanks!
I don't think that would work; per the docs the |
There was a problem hiding this comment.
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 comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@python/organization-owners please block. A |
|
Hiding with the correct hide reason (spam) @python/organization-owners please block as spam, brand new account that just spams random projects. Also reported to GitHub; hopefully they'll deal with it quickly like they have done a great job with other similar spammers lately. |
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