[MERGE] Improvements to bzr added|modified

Ian Clatworthy ian.clatworthy at internode.on.net
Mon Apr 28 09:20:26 BST 2008


Adrian Wilkins wrote:
> Standard output behaviour for "added" and "modified" made consistent
> with "unknowns".
> Null character separation option added to "added" and "modified" (but
> not "unknowns" because there is an equivalent "bzr ls --unknown --null"
> 
>  * "added" and "modified" now use osutils.quotefn as "unknown" does
>  * Added missing blackbox test for "modified"
>  * Added code that improves test coverage of
>    * Path quoting behaviour for "added","modified","unknowns"
>  * Tests also verify new --null behaviour for "added","modified"

Adrian,

Thanks for this. This is arguably ready to merge (after some tweaks) and
the code itself is good and clean. The reason I'm voting resubmit
(instead of tweak) is because the main change I want is more than a few
lines.

bb:submit

Tweak 1 is a simple one: please add an entry to NEWS explaining the
improvement you've made and give yourself credit.

Tweak 2 is more complex: the test code needs to be "dumbed down". By
that, I mean make the test logic and comparisons as simple as you can.
Right now, the test code *feels* like it copies much of its logic from
the code it's testing. That can be fine, but it can also mean that bugs
slip though. In a nutshell, it "smells" a little when the core code is
less complex than the test code. If you're interested, see
http://xunitpatterns.com/Obscure%20Test.html for a general treatment of
this topic.

To give a concrete example, avoid using osutils.quotefn() in your
testing and test against the expected result directly. (Imagine if
quotefn was buggy - both the code and test would match and both would be
wrong.) To give another, avoid calling the one check_*() method with an
"if null:" statement inside it. Either use two check moethds or, better
still, test against the various expected output strings as directly as
you can "inline".

Sorry if the above comes across too negatively - it's *meant* to be
constructive feedback, though the difference is often lost across email.

Ian C.



More information about the bazaar mailing list