Skip to content
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

Several PEPs: Use explicit :pep: and :rfc: roles #2209

Merged
merged 4 commits into from
Jan 21, 2022

Conversation

AA-Turner
Copy link
Member

@AA-Turner AA-Turner commented Jan 3, 2022

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)? or PEP\s+\d+ for PEPs, and RFC(-|\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 a pep-nnn pattern.

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 current pep2html based 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

@CAM-Gerlach
Copy link
Member

I think the best way forwards for this PR is likely to split it up.

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.

@CAM-Gerlach 3d45cb1 (#2209) enables putting a :pep:N reference in the resoultion field -- if you're OK with that I can go ahead with it as a small one before splitting out the RFC and markup fixes.

I saw that; makes sense to me to allow it.

@AA-Turner AA-Turner force-pushed the pep-676-explicit-roles branch from fb18a0c to fe00d69 Compare January 21, 2022 00:51
@AA-Turner
Copy link
Member Author

I saw that; makes sense to me to allow it.

Done

@AA-Turner
Copy link
Member Author

re-opening it again as another PR is going to ping all the codeowners again

True. There are 3 somewhat distinct changes here:

  • :pep: role (382 files)
  • :rfc: role (33 files)
  • fix incorrect grave accents [backticks] (16 files)

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

@CAM-Gerlach
Copy link
Member

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 could try deleting CODEOWNERS on the PR and then dropping that commit).

I don't think that would work; per the docs the CODEOWNERS from the target branch is used, not the one in the PR source branch, so you'd actually have to merge the change deleting CODEOWNERS to main, open your PRs and then revert the previous change.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

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.

pep-0001.txt Show resolved Hide resolved
pep-0001.txt Outdated Show resolved Hide resolved
@AA-Turner AA-Turner force-pushed the pep-676-explicit-roles branch from fe00d69 to 5e7e3f5 Compare January 21, 2022 10:57
@AA-Turner AA-Turner changed the title PEP 676: Implementation updates pt 2 (explicit roles) Use explicit :pep: and :rst: roles Jan 21, 2022
@AA-Turner AA-Turner changed the title Use explicit :pep: and :rst: roles Several PEPs: Use explicit :pep: and :rst: roles Jan 21, 2022
@AA-Turner AA-Turner changed the title Several PEPs: Use explicit :pep: and :rst: roles Several PEPs: Use explicit :pep: and :rfc: roles Jan 21, 2022
@AA-Turner AA-Turner merged commit 113e490 into python:main Jan 21, 2022
@AA-Turner AA-Turner deleted the pep-676-explicit-roles branch January 21, 2022 11:03
@AA-Turner AA-Turner restored the pep-676-explicit-roles branch January 21, 2022 11:04
@AA-Turner
Copy link
Member Author

Thanks @CAM-Gerlach; will tackle link title text in a follow-up change

A

@CAM-Gerlach
Copy link
Member

Thanks @AA-Turner !

@Googleclo

This comment was marked as off-topic.

@Googleclo

This comment was marked as off-topic.

@Googleclo

This comment was marked as off-topic.

@AA-Turner
Copy link
Member Author

@python/organization-owners please block.

A

@CAM-Gerlach
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants