[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