Repository navigation
Clarify force-push usage for CPython PRs #579
Description
As someone who regularly reviews PRs from contributors with varying levels of experience with CPython's workflow, often times I find that they repeatedly squash old commits and force-push to the PR branch each time a new commit or round of commits are added (after it has been opened). In most cases, this makes the history of the PR much more difficult to follow for review purposes.
I assume that their intention is to "clean up" the history and leave a single commit at the end for easier merging, but in reality this doesn't do much to help since core devs typically make use of GitHub's "Squash and Merge" feature or automerge via miss-islington to squash the commits.
Guido mentioned this last year in https://discuss.python.org/t/pep-601-forbid-return-break-continue-breaking-out-of-finally/2239/42, but I just thought about it again after seeing it happen "out in the wild" recently across CPython PRs from different contributors.
It is mentioned at the end of section 3.8, but I think it could be made a bit more obvious since this seems to occur rather frequently, and can be disruptive to the PR review process. It's a fairly small, but repeated cost to core development resources that could be avoided.
One idea I had in mind was adding a note to the "Quick Guide", in section 3.2, which is likely what many contributors primarily consult and regularly refer back to:
Current:
Here is a quick overview of how you can contribute to CPython:
Create an issue that describes your change [*]
Create a new branch in Git
Work on changes (e.g. fix a bug or add a new feature)
Run tests and make patchcheck
Commit and push changes to your GitHub fork
Create Pull Request on GitHub to merge a branch from your fork
Review and address comments on your Pull Request
When your changes are merged, you can delete the PR branch
Celebrate contributing to CPython! :)
Recommended:
Here is a quick overview of how you can contribute to CPython:
Create an issue that describes your change [*]
Create a new branch in Git
Work on changes (e.g. fix a bug or add a new feature)
Run tests and make patchcheck
Commit and push changes to your GitHub fork
Create Pull Request on GitHub to merge a branch from your fork
Review and address comments on your Pull Request (without force-pushing)
When your changes are merged, you can delete the PR branch
Celebrate contributing to CPython! :)
Any other recommendations for locations that it could be mentioned would also be welcome, but that was the most impactful that I had in mind.
Activity
[-]Clarify force-pushing usage for CPython PRs[/-][+]Clarify force-push usage for CPython PRs[/+]DahlitzFlorian commentedon May 21, 2020
In section 3.8 this is already mentioned:
However, I agree with you that it would be nice to have a note somewhere at the beginning so it is an eye catcher for contributors and more likely to be seen.
aeros commentedon May 21, 2020
Yep, that was my main point, which is why I linked to that section in 3.8 in my original post. :-)
It's easy for sections like that to get missed by new contributors taking a quick glance at the devguide, and it seems to happen often enough for it to be worthwhile to clarify.
DahlitzFlorian commentedon May 21, 2020
Are you working on a PR or can I take this one?
aeros commentedon May 21, 2020
@DahlitzFlorian You can take this one if you'd like. Also, feel free to @ mention me in the PR, and I'll try to get around to reviewing it within a reasonable period.