On Mar 25, 2015, at 01:02 PM, Stephen J. Turnbull wrote: >(1) Suppose you change a small function whose formatting is not PEP 8 > conformant (or otherwise so ugly you can't help fixing it -- of > course, check "blame" first, if Barry committed those lines, have > your eyes checked instead :-). I would (1) fix the problem in the > current style, commit, and then (2) reformat the function to PEP > 8, and give that a separate commit. If the number of lines that > would *not* be in the diff at all if you didn't mess with style is > greater than the number of lines in the diff from step (1), don't > do step (2) without checking with the project leads.
+1; of course, I'm happy to look at purely style fixing branches, again if they're small, concise, and manageable. If I can't tell that only style was changed (i.e. no features or bug fixes snuck in) then I'd probably reject it. My general approach for style nits is: - If they're pervasive (e.g. lots of too-long lines), I'll move the mp to Needs Fixing and ask you to repair them. - If they're fairly minor or rare, I'll comment about them in the mp and fix them up when I merge. I don't want to be so nitpicky as to reject a good branch just for some minor style issues, but I *do* want the submitter to get a sense of how the next branch can avoid those issues. >(2) Documentation and test changes implied by your code changes should > go in the same branch. In other projects I usually commit docstring > changes with the code change, and use two more commits, one for > standalone docs and one for unit tests. +1. This is actually something I wish vcses would handle better. Maybe there's git magic that would make this workflow better, but here's what I do when reviewing a merge proposal (most relevantly, bug fixes): - Run the full test suite with the full branch merged, but not yet committed. If there are *any* failures, the mp gets Needs Fixing'd. - Unapply the fix, and run the test suite. I should now see the new tests, and only the new tests fail. - Review the tests; do they actually test what they claim, and do they actually test the new code? - Reapply the fix. Everything should pass again. I can do this with `bzr shelve` but it's a little clunky. It would be kind of cool if TDD could be evident somehow in the branch, maybe e.g. as separate commits. `git rebase --interactive` might be promising. >(3) Unrelated documentation changes (eg, a typo you notice in another > function's docstring, or you want to add a docstring to an > undocumented function that you had to study) should go in a > separate *branch*. I usually have a branch just to collect these > small improvements, and push them (or submit a merge request, > depending on the project workflow) all at once. +1 >These practices are noticably tedious for the committer, but I find >they greatly improve the usefulness of commit logs and the readability >of diffs. It's not officially in the Zen of Python, but widely >acknowledged, that Python should be written with the assumption that >the code will be read many thousand times more often than it's written. Absolutely. Really great advice, Steve. Cheers, -Barry
pgpurSTtYwHB4.pgp
Description: OpenPGP digital signature
_______________________________________________ Mailman-Developers mailing list Mailman-Developers@python.org https://mail.python.org/mailman/listinfo/mailman-developers Mailman FAQ: http://wiki.list.org/x/AgA3 Searchable Archives: http://www.mail-archive.com/mailman-developers%40python.org/ Unsubscribe: https://mail.python.org/mailman/options/mailman-developers/archive%40jab.org Security Policy: http://wiki.list.org/x/QIA9
