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

[Guillermo Gonzalez]

On Mon, Mar 31, 2008 at 10:09 AM, Aaron Bentley <aaron at aaronbentley.com> wrote:
[snip]
Aaron,
   First of all, thanks for the extensive review :)
>
>  > +    def show_properties(self, properties):
>  > +        """Displays the custom properties returned by each registered handler.
>  > +
>  > +        If a registered handler raise an error, it's silently logged and the
>  > +        next handler in the registry is executed.
>  > +        """
>
>  Shouldn't that be considered a bug in the handler?  What kinds of errors
>  are you saying should be ignored?
>

I agree that it should be considered a bug, but I wasn't sure if
propagating the error and stop the log for a broken handler or keep
executing log.

should the log stop if a handler is broken?

>  Also, your docstring should have "raises", not "raise".

apologize me for the typo, I'll fix it.

>
>  > +            try:
>  > +                for key, value in handler(filtered_props).items():
>  > +                    self.to_file.write(key + ': ' + value + '\n')
>  > +            except:
>  > +                mutter('custom property handler: %s raised an error',
>  > +                        key)
>  > +                trace.log_exception_quietly()
>
>  Bare except clauses are almost always a bad idea.  For instance, this
>  will ignore a KeyboardInterrupt or a pipe error.

Thanks for the tip.
How should it handle this errors? (I don't know exactly what errors a
handler can raise, actually it can be any python or bzr related error)

>
>  > === modified file 'bzrlib/tests/blackbox/test_log.py'
>  > --- bzrlib/tests/blackbox/test_log.py 2008-01-10 22:34:09 +0000
>  > +++ bzrlib/tests/blackbox/test_log.py 2008-03-31 03:56:49 +0000
>  > @@ -184,6 +184,56 @@
>  >          self.assertNotContainsRe(log, r'revno: 1\n')
>  >          self.assertContainsRe(log, r'revno: 2\n')
>  >          self.assertContainsRe(log, r'revno: 3\n')
>  > +
>
>  These don't look like UI tests to me.  They should probably be in
>  bzrlib/tests/test_log, instead.

I didn't added those three asserts, but I don't have any problem to move them.

>
>  > +    def test_log_without_custom_properties_handler(self):
>  > +        tree = self._prepare()
>  > +        tree.commit(message='', revprops={'first_prop':'first_value'})
>  > +        log = self.run_bzr("log --limit 1")[0]
>  > +        self.assertNotContainsRe(log, r'first_prop: first_value\n')
>  > +        log = self.run_bzr("log -r1")[0]
>  > +        self.assertNotContainsRe(log, r'first_prop: first_value\n')
>
>  ^^^ How does this ensure that there are no custom properties handlers?
>

ups, I missed that one. I keep forgetting that selftest load plugins.
the test assume the registry is empty (my mistake), but it should
clean the registry or replace the registry with an empty one.

I'll work on this issues and resubmit.

Thanks for the comments and guidance.

Cheers,

-- 
Guillermo