[python-committers] Regarding reviewing test cases written for tabnanny module

[Nick Coghlan]

On 12 April 2017 at 00:21, R. David Murray <rdmurray at bitdance.com> wrote:
> On Mon, 10 Apr 2017 21:53:44 -0700, Guido van Rossum <guido at python.org> wrote:
>> - When the contributor makes multiple local commits without pushing to the
>> PR, I recommend using --amend unless they have several commits that
>> actually are logically distinct and relevant to the reviewer. (--amend is
>> especially important when fixing lint bugs or typos).
>>
>> - Please don't use --amend across pushes to the PR
>>
>> - It's OK to just pull+rebase and then use git push -f, but honestly
>> pull+merge is fine too
>>
>> - When responding to a review please NEVER use commit --amend, that's where
>> reviewing becomes painful
>
> In the devguide PR addressing this issue, I suggested we just say that
> one should never force push to the PR.  I can see that a force
> push after a rebase *before* addressing review comments would be fine,
> but maybe it would be better to prefer merge since we're going to
> squash at the end anyway.

Yeah, Serhiy made this point when we were working on the instructions
for committers editing a contributor's PR branch:
https://docs.python.org/devguide/gitbootcamp.html#editing-a-pull-request-prior-to-merging

Since we do the squash & merge to get an atomic commit at the end, it
doesn't make sense to do any force pushes along the way.

>> - It's up to the reviewer who merges to rewrite the commit message: the
>> reviewer usually has a much better sense for what info in the commit
>> message will still be interesting a month or a year from now than the
>> contributor. (Often just copying the original comment from the top of the
>> PR is adequate.)
>
> Some of our core committers need to learn to do this :)

Technically coming up with a meaningful message when committing
patches isn't a new requirement, but I admit it's a lot easier to
forget to clean things up when there's a big green button right in
front of you waiting to be pushed :)

Cheers,
Nick.

-- 
Nick Coghlan   |   ncoghlan at gmail.com   |   Brisbane, Australia