[MERGE][Bug #162469] log displayers for custom revision

[Guillermo Gonzalez]

Hi,

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
> 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.

done.

>
>> +    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 :)
done.

>
>> +        If a registered handler raises a error it is propagated.
>
> s/a error/an error,/

done.

>
>> +        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
fixed now.

>
> 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.

Cheers,

--
Guillermo
-------------- next part --------------
A non-text attachment was scrubbed...
Name: properties_log-2.patch
Type: application/octet-stream
Size: 19863 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080709/fc7b527c/attachment-0001.obj