[Mailman-Developers] Getting "approval" to go ahead
Barry Warsaw
barry at list.org
Wed Mar 25 05:22:17 CET 2015
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://mail.python.org/pipermail/mailman-developers/attachments/20150325/2143df32/attachment.sig>
More information about the Mailman-Developers
mailing list