[MERGE][Bug #162469] log displayers for custom revision
guillo.gonzo at gmail.com
Wed Jul 9 21:20:21 BST 2008
Ian , thanks for the detailed review, my comments below.
And the fixed patch is attached
On Wed, Jul 9, 2008 at 3:37 AM, Ian Clatworthy
<ian.clatworthy at canonical.com> wrote:
> Guillermo Gonzalez wrote:
>>> Any comments are more than welcome.
> One coding bug and some minor doc things to clean-up. Another test requested
>> + * New registry for custom log displayers and the method in
>> + LongLogFormatter to display the custom properties returned by the
>> + registered handlers. (Guillermo Gonzalez, #162469)
> This NEWS item is fine but I'd prefer to see it under IMPROVEMENTS instead of
>> + Plugins can register functions to show custom revision properties using
>> + the custom_properties_handler_registry. The registered function
> You mean properties_handler_registry here, not custom_properties_handler_registry.
Indeed, I just added "grep after rename" to my pre-send patches policy :)
>> + If a registered handler raises a error it is propagated.
> s/a error/an error,/
>> + for key, handler in properties_handler_registry.iteritems():
>> + for key, value in handler(revision).items():
>> + self.to_file.write(indent + key + ': ' + value + '\n')
> I wonder if you ought to be displaying these in sorted order. Any thoughts?
Maybe the handlers could return a list of tuples instead of dict, so
the ordering of the properties is defined by the handlers.
should that be ok?
>> +class TestCaseWithoutPropsHandler(TestCaseWithTransport):
>> + def setUp(self):
>> + super(TestCaseWithoutPropsHandler, self).setUp()
>> + # keep a reference to the "current" custom prop. handler registry
>> + self.properties_handler_registry = \
>> + log.properties_handler_registry
>> + # clean up the registry in log
>> + log.properties_handler_registry = registry.Registry()
>> + def _cleanup(self):
>> + super(TestLongLogFormatter, self).tearDown()
> That last line looks wrong to me. The class ought to be TestCaseWithoutPropsHandler, yes?
> And you need to be calling _cleanup() or overriding tearDown(), yes?
absolutely right :)
my fault, copy & paste mess :P
> The tests look OK though I'd like another one that actually depends on
> the revision object being correctly passed to the handler. As best I can
> tell, we could be passing any object into the handler and the tests would
> still pass.
I added a test for this
TestLongLogFormatter.test_properties_handler_bad_argument , please let
me know if it's the right way to test this case, or if you know of a
similar one in bzrlib.tests I can use as a base.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 19863 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080709/fc7b527c/attachment-0001.obj
More information about the bazaar