osdir.com

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Python-Dev] I reverted "Add Windows App Store package" change


On 07Dec2018 1340, Terry Reedy wrote:
> Simple bugfix example:
> <commit 1> Add test to test_mod that fails with TwinkleError.
> Posted to issue by Joe Blow.
> <commit 2> Make new test pass using the 'underhand' strategy.
> 
> The split above is not really necessary, but PR 10245 squashed changes 
> to 52 files of 15 file types into one initial commit.
> 
> https://github.com/python/cpython/pull/10245/commits/17ba155a7b794926ce705ee0e2af787fbd2996e6 
> 
> 
> View Files displays them alphabetically.? Jump to ... lists them in the 
> same order, but includes the functions changed, so it is hundreds of lines.
> 
> I think this PR would have benefited from having perhaps 10 or more 
> initial commits, each with a comment about the group of files included. 
> In icon commit could have said something about the source and purpose of 
> the added icon files.? One commit could have included the venv and test 
> changes that you want to review.? An added message, as long as needed, 
> could have explained these particular changes.
> 

This is great in theory, but if you look back at the original PR it 
would have been 100+ commits (I was occasionally squashing and force 
pushing). What you're really proposing is:

* do all the work using the git workflow (stream-of-consiousness commits)
* squash all the commits at the end
* re-expand the single commit into logical groupings of files and 
re-commit them

So it's easy to sit back and imagine that I had all the perfect changes 
ready to go and deliberately chose to make it harder to review, but 
that's not the case at all. Making it sound like this is how development 
works is really disparaging to people who find themselves not producing 
perfect commit histories.

And let's be honest, there's no good tooling for turning a series of 
interdependent commits into a smaller set of sensible ones. Squashing at 
least gets rid of the changes that were reverted as part of the entire 
PR, and if you then just want to split by file you're really just asking 
for extra work. If someone had bothered to say "I'll review the parts of 
it that are relevant to me if you can split them out" then I would have, 
but nobody (in public) showed any interest in reviewing the changes at 
all - I had some private reviews done by colleagues at work, who weren't 
so demanding about it.

I review as many PRs as I send these days (maybe more? I'm not 
counting), and I always try to make an effort to have mercy towards 
people to save them having to work extra hard just to make my life a 
little bit easier. It grates to have had an incremental change visible 
in public for over two months, have it be totally ignored the entire 
time, and then have to defend something that I already did. Luckily I 
get paid work time for doing this; really can't see why I'd want to 
suffer through this for free...