10000 Clarify force-push usage for CPython PRs · Issue #579 · python/devguide · GitHub
[go: up one dir, main page]
Skip to content

Clarify force-push usage for CPython PRs #579

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.

Sign up for GitHub

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

Jump to bottom
Closed
aeros opened this issue Mar 21, 2020 · 4 comments · Fixed by #588
Closed

Clarify force-push usage for CPython PRs #579

aeros opened this issue Mar 21, 2020 · 4 comments · Fixed by #588
Assignees
@aeros

Comments

@aeros
Copy link
Contributor
aeros commented Mar 21, 2020 •
edited
Loading

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.
@aeros aeros self-assigned this Mar 21, 2020
@aeros aeros changed the title Clarify force-pushing usage for CPython PRs Clarify force-push usage for CPython PRs Mar 21, 2020
@DahlitzFlorian
Copy link
Contributor

In section 3.8 this is already mentioned:

Please keep the commit history in the pull request intact by not squashing, amending, or anything that would require a force push to GitHub.

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
Copy link
Contributor Author
aeros commented May 21, 2020

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.

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
Copy link
Contributor

Are you working on a PR or can I take this one?

@aeros
Copy link
Contributor Author
aeros commented 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.

@DahlitzFlorian DahlitzFlorian mentioned this issue May 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aeros aeros

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add a note about not modifying the git history to the quick guide DahlitzFlorian/devguide
2 participants
@DahlitzFlorian @aeros
0