[MERGE][Bug #162469] log displayers for custom revision
Jelmer Vernooij
jelmer at samba.org
Fri Jul 4 11:45:22 BST 2008
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?
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.
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
More information about the bazaar
mailing list