Explain why commits in a PR shouldn't be squashed. by Mariatta · Pull Request #162

Explain why commits in a PR shouldn't be squashed. by Mariatta · Pull Request #162 · python/devguide

Choose a reason for hiding this comment

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

A couple of minor grammar nits, but LGTM.

		spelling mistake), then the pull request needs to have the "trivial" label
		added to it.

		If your pull request involves several commits, as a result of addressing code

Choose a reason for hiding this comment

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

The comma after "commits" is unnecessary.


		If your pull request involves several commits, as a result of addressing code
		review comments, please do not squash them. When you squash your commits,
		reviewers lose the ability to view the diff of one commit to the next,

Choose a reason for hiding this comment

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

The comma after "next" is unnecessary.

		If your pull request involves several commits, as a result of addressing code
		review comments, please do not squash them. When you squash your commits,
		reviewers lose the ability to view the diff of one commit to the next,
		and therefore unable to easily verify whether their comments have been addressed.

Choose a reason for hiding this comment

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

I think there's an "are" missing between "and" and "therefore".

Choose a reason for hiding this comment

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

Some place I think should be more clarify.

		added to it.

		If your pull request involves several commits as a result of addressing code
		review comments, please do not squash them. When you squash your commits,

Choose a reason for hiding this comment

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

I think please do not squash them should be more emphasize with blod and capitalize. And perhaps append "(that is, please do not git rebase then git push --force all your commit)" to clear what step will squash the commit.

Choose a reason for hiding this comment

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

git commit --amend; git push --force also does a squash (and may be the more common reason that people squash). I'd prefer that we use a positive form of wording, something like "please keep the full history of your PR intact (i.e. don't "squash" or amend the commits, but keep them separate)"

Choose a reason for hiding this comment

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

I like using the positive language, it gets across the reason better. Perhaps it would be sufficient to say "don't do anything that would require a force push" in that parenthetical?

Choose a reason for hiding this comment

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

That seems reasonable.

Choose a reason for hiding this comment

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

Thanks @pfmoore and @bitdancer. I updated this. Please check it this sounds better.

		review comments, please do not squash them. When you squash your commits,
		reviewers lose the ability to view the diff of one commit to the next
		and are therefore unable to easily verify whether their comments have been
		addressed.

Choose a reason for hiding this comment

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

At the end, should it provide the describe that the member who contribute with PR, when approved the PR, he/she will finish with the squash of the commit.

Choose a reason for hiding this comment

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

Thanks @lulouie :) Updated with this info.

Choose a reason for hiding this comment

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

A few minor suggestions. Looks good though.

		Your pull request may involve several commits as a result of addressing code
		review comments. Please keep the commit history in the pull request intact by
		not squashing, amending, or anything that would require a force push to GitHub.
		This allows reviewers to view the diff of one commit to another so they can

Choose a reason for hiding this comment

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

perhaps: This -> A detailed commit history allows ...

Choose a reason for hiding this comment

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

Thanks :) Updated.

LGTM now. Ironically, I couldn't work out how, in the github UI, to see the revised version of the wording I'd commented on, short of going to the full "Files changed" tab and looking at the final wording of the whole section. Maybe that's just because there's only one change hunk in this PR, though.

I would rewrite that paragraph as follows:

When addressing review comments, please do so by making and pushing
new commits. If you want to re-sync with the master branch, do a merge
commit and push that. A detailed commit history allows reviewers to view
the diff of each commit to easily verify whether their comments have been
addressed. The commits will be squashed when the pull request is merged,
so in the master branch the entire PR will appear as a single commit.
The rule of thumb to follow is: don't do any operation what would require
a force push to your PR.

@pfmoore: there are links to the individual commits in the comment stream. If you can't find them there, you can also get to them through the 'commits' tab near the top of the page.

@bitdancer Ah I see. That's not very obvious, but as I say maybe that's because this is a pretty small change (and just seeing the whole rework is actually fine). I doubt it'd be a problem with a bigger change.

I'm going to merge this as written so the "force push" information gets to the reader sooner vs. later. We can iterate on word choice and tone in follow-on PRs. Thanks everyone.

AA-Turner pushed a commit to AA-Turner/devguide that referenced this pull request

Jun 17, 2022

- Guide contributor to select a workflow which avoids force push

Closes python#159