[MERGE][Bug #162469] log displayers for custom revision
Ian Clatworthy
ian.clatworthy at canonical.com
Wed Jul 9 07:37:07 BST 2008
Guillermo Gonzalez wrote:
>> Any comments are more than welcome.
One coding bug and some minor doc things to clean-up. Another test requested
too.
bb:resubmit
> + * 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
BUGFIXES.
> + 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.
> + 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?
> +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?
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.
Ian C.
More information about the bazaar
mailing list