Skip to content

Meta: Add templates/checklists for new, Accepted/Rejected & Final PEPs #2956

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

Merged
merged 13 commits into from
Feb 1, 2023

Conversation

CAM-Gerlach
Copy link
Member

As discussed in #2937 , this adds new templates for key PEP state transitions with concise checklists for each of the steps needed, to help both authors and reviewers. This relies on a new, experimental/alpha GitHub feature (enabled only on our repo) to work, so hopefully this is doing it right, but the only way to know is to merge this.

If it works, we can follow this up with a couple more templates for the other two common cases (updating a draft PEP and changing an old PEP), which don't need a checklist or more text, but can contain additional reminders tailored to that situation and be an opportunity to inform users what types of PRs are welcome and redirect/point them to more resources.

@CAM-Gerlach CAM-Gerlach added the meta Related to the repo itself and its processes label Jan 9, 2023
@CAM-Gerlach CAM-Gerlach requested a review from a team as a code owner January 9, 2023 20:06
@CAM-Gerlach CAM-Gerlach self-assigned this Jan 9, 2023
Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

Here are some thoughts and attempts to simplify things.

@CAM-Gerlach
Copy link
Member Author

Sorry for the delay; I started to go over this several days ago but accidentally marked the notification as "done" instead of "unread", and so it slipped through the cracks for several days. In addition to the suggested revisions above (with some adaptations), and a couple inspired by them, I pushed another commit re-organizing and revising the new PEP checklist more thoroughly to account for Standards Track vs. other types of PEPs, as well as tweaking the other checklists to clarify this too

I also attempted to address both of Adam/Petr's two points above, as well as fixed/revised a few other things I noticed. Looking forward to your further feedback!

@encukou
Copy link
Member

encukou commented Jan 17, 2023

Thanks! Tooks good to merge now!

I still think the two comments and intervening heading is superfluous though. Shouldn't “Make sure to include the PEP number in the pull request title” be a checklist item too?

@CAM-Gerlach
Copy link
Member Author

Thanks, that makes sense. I've gone ahead and removed the first comment block entirely on all the templates (except for other), added the relevant steps (PR title, reading PEP 1/12) to new or existing checklist items, and also slimmed down the second comment block on each.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thanks!

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Jan 19, 2023

Based on @encukou 's previous points above and Brett's comment on #2966 , I renamed the existing "Other" template to be for "Update/modify PEP", greatly slimmed down the HTML comment and moved the "PR title" to be a single-line checkbox rather than a multi-line comment, which makes it take up less space but be more easily visible. Furthermore, I added a set of checkboxes for the types of changes that are welcomed: to a draft PEP, to an accepted/final PEP with SC approval, or resolving a purely technical or editorial defect. Overall, this makes it much more visible and useful, while actually taking up several lines less than the original.

I also added a much more minimal "Other" template for infra, meta, etc. PRs with just a two-line reminder comment to check the contributing guide and make sure this repo is the right place for the change, which makes it a fraction the length of the original and cuts out material not relevant to that type of PR (PEP number in title, etc).

Finally, I was able to trim a few more lines from the comments in the other templates, without any substantive affect on the content.

The new Update/modify PEP template renders like this:


  • Change is either:
    • To a Draft PEP
    • To an Accepted or Final PEP, with Steering Council approval
    • Resolving a purely technical or editorial issue
  • PR title prefixed with PEP number (e.g. PEP 123: Summary of changes)

@CAM-Gerlach
Copy link
Member Author

As I noticed Hugo using it on python/core-workflow#491, I've added a title: field to the appropriate templates, which pre-fills the title following the given format, which should help a lot with following the standard title format. Of course, it might not actually work yet, but same with all the other fields, only one way to find out...

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Thanks! I think we're good to merge (and iterate if need be)?

@CAM-Gerlach
Copy link
Member Author

Due to it specifically coming up on the past two new PEPs, and the most recent new PEP having an issue where the listed sponsor didn't actually approve being listed, I've added back the explicit checklist item for new PEPs to either have a core dev/PEP editor as an author, or formally confirmed as a sponsor.

@hugovk
Copy link
Member

hugovk commented Feb 1, 2023

Think we're good to merge this, we can always adjust later as needed.

Thank you!

@hugovk hugovk merged commit f009036 into python:main Feb 1, 2023
@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Feb 1, 2023

Okay, I'm glad I was around to check if it actually worked, as I had no idea if it would and was hoping to be around when this was merged—I had very limited info from GitHub, since it's all super experimental.

Indeed, it does work in a basic sense, but it looks like the current UI just shows the file names, and the data block at the top of the templates is not used currently like it is for the issues:

image

With that in mind, we presumably should just remove the data block for now and rejigger the file names so they're more informative, at least as a quick stopgap. PR in work...

@hugovk
Copy link
Member

hugovk commented Feb 1, 2023

You also don't get the selector when doing "New pull request" from https://github.com/username/peps/branches, but I guess that's a less common path than clicking the link from https://github.com/python/peps for a recently pushed branch.

@CAM-Gerlach
Copy link
Member Author

Hmm, good point—I didn't even realize you could do that, I always just use the recently pushed branch link or set it up manually. That's possibly because the "compare" UI (which the template selector is on) never triggers when going that route, and it instead jumps straight to the PR creation. We can mention that to GitHub.

I've opened #2998 to fix the issues mentioned here; hopefully we can get that merged quickly to avoid any impact on contributors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Related to the repo itself and its processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants