Repository navigation
Clarify issue/PR choreography #1815
Description
Activity
Hi!
I would love to help clarify the issue/PR choreography in the devguide as suggested by @picnixz.
The goal would be to update the guide in
getting-started/pull-request-lifecycle.rstto clearly distinguish:- Trivial fixes (e.g. typos, small doc fixes) where an issue is not strictly required.
- Non-trivial bugs/features where contributors should wait for triager validation before opening a PR.
- How to check for existing discussion or consensus before writing automated or large PRs.
Could you please assign this to me, or give me the green light to draft a PR for this?
I think we still need to decide what counts as trivial and non-trivial. For instance, for typos/small doc fixes, we don't want typo fixes only in non-user-facing code. But, let's say I have a typo in a .rst file about some function. Then it's better to go through the entire file to fix typos altogether. And if it's in the .rst docs, it's also good to look at the corresponding docstrings / C docs (those user-facing) to synchronize them. So what started as an "easy" fix requires a bit more work. And this is something I would like to see in the devguide.
It's always easy to open an issue after the fact (we don't have a meta issue for typos but we have one for fixing rst references for instance) but telling users what is simple and how to fix that simple task is delicate.
About waiting for triaging, I think it's a simple sentence to add (but we should decide where to add it, though you can open a PR for this specific part). But it may be wrong to say "just wait" because sometimes it's obvious that there is an issue. Most of the time, we want contributors to wait:
- if the OP said they will open a PR (that's rude to open a PR for someone else's issue)
- if it's not clear whether it's a bug or not (users tend to report bugs when it's more a feature or vice-versa). But that is hard to explain in the devguide
- when issue is old: just don't pick an old issue without readnig the history of the issue. If there is no clear consensus it's in general a bad idea to open a PR. And commenting "i want to help, so I'm going to create a PR" when the discussion didn't advnace is not really the best idea (reviving a discussion is harder than what most people think, status quo is sometimes th best)
- and likely more situations that I forgot
I plan to discuss this on the following CPython core dev sprint but we should first discuss about what to write.
Another aspect of the PR lifecycle I don't understand myself: do backports need their own reviews and approvals, or can the author of the original PR merge the backports on their own?
Backports:
- I merge my own changes as if I were writing the PR (since I did write the original PR). I do review it myself quickly (if I know about the code it's easy to remember "ah yes this won't work"). But if something fails (like the CI) or I needed to cherry-pick the commit and solve conflicts manually (and it's not just a trivial change), better check manually for edge cases.
- When we're in the RC phase, this rule is different though (cc @hugovk). During RC, you need the approval of someone else (not necessarily the RM).
Reacted by Stan Ulbrych and Hugo van KemenadeIn general, I think this isn't written down because ultimately it's an individual choice. I merge automatic backports without another review, but in the case of conflicts it depends. How confident am I in reviewing my own changes, if I had to touch something I'm not particularly happy with (like Windows build configuration), I'd ask for another review from an expert.
In general, I think this isn't written down because ultimately it's an individual choice. I merge automatic backports without another review, but in the case of conflicts it depends. How confident am I in reviewing my own changes, if I had to touch something I'm not particularly happy with (like Windows build configuration), I'd ask for another review from an expert.
Adding this (or something like it) to the devguide is better than no mention of what to do.
I presume this information has usually been passed down through mentoring or picked up through observation, at least, that's how I learned. That said, if it's not clear to core developers, documenting it sounds worthwhile. Happy to review a PR.
I plan to first discuss this in person at the next sprint and I do not think this is something for a new contributor. This requires a core dev to draft the project IMO.
From a discussion with @picnixz:
This might be a duplicate of other feedback here in issues, but I wanted to capture it before going back to $WORK.