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

[Jelmer Vernooij]

Am Freitag, den 04.07.2008, 16:18 -0300 schrieb Guillermo Gonzalez:
> On Fri, Jul 4, 2008 at 7:45 AM, Jelmer Vernooij <jelmer at samba.org> wrote:
> > Hi Guillermo,
> >
> > Am Freitag, den 04.07.2008, 11:12 +0200 schrieb Jelmer Vernooij:
> >> This merge request appears to be dead - do you still intend to work on
> >> it?
> yes, I still want to work on this and try to merge it into the core.
Cool :-)

> >
> > I played with your patch a little bit to fix
> > https://launchpad.net/bugs/161830. Would it be possible to pass the full
> > revision object rather than just its properties member? bzr-svn would in
> > particular need the revision id so it can parse that and do sqlite
> > lookups based on it.
> 
> Absolutely, :-)
> I can spend some time on this during this weekend, basically I need to
> rework the error handling bit (any ideas are welcome ;-) )
Is there any error handling required there at all? I think it would be
sane to just require that plugins don't raise exceptions (barring bugs,
etc, of course). We don't have similar code for other hooks as far as
I'm aware.

Cheers,

Jelmer


> 
> >> Am Montag, den 31.03.2008, 23:17 -0300 schrieb 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,
> >
> > --
> > Jelmer Vernooij <jelmer at samba.org> - http://samba.org/~jelmer/
> > Jabber: jelmer at jabber.fsfe.org
> >
-- 
Jelmer Vernooij <jelmer at samba.org> - http://samba.org/~jelmer/
Jabber: jelmer at jabber.fsfe.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 315 bytes
Desc: Dies ist ein digital signierter Nachrichtenteil
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080704/e9310040/attachment-0001.pgp