Code review comment for lp:~maria-captains/bzr/bugtracker

Hi, Jelmer!

On Mar 21, Jelmer Vernooij wrote:
> Hi Sergei,
>
> Thanks for working on this. Please find some inline comments below:
>
> > === modified file 'bzrlib/bugtracker.py'
> > --- bzrlib/bugtracker.py 2011-02-25 12:12:39 +0000
> > +++ bzrlib/bugtracker.py 2011-03-21 09:20:55 +0000
> > @@ -145,6 +145,23 @@
> > return tracker.get_bug_url(bug_id)
> >
> >
> > +def get_bug_id(branch, bug_url):
> > + """Return a bug id corresponding to the bug url.
> > + """
> > + for tracker_name in tracker_registry.keys():
> > + tracker = tracker_registry.get(tracker_name)
> > + # not all bug trackers may support reverse engineering
> > + if hasattr(tracker, 'reverse_engineer'):
> > + bug_id = tracker.reverse_engineer(url, branch)
> > + if bug_id is not None:
> > + return bug_id
> Please avoid the use of "hasattr" as it "eats" exceptions. It's usually
> better to use getattr() with the default argument, e.g.:
>
> if getattr(tracker, "reverse_engineer", None) is not None:

Fixed.

> > + # raising an error here corrupts the tree in a way that
> > + # is not obvious to recover from. As its only value is to help
> > + # the user to fill in the bug number on the next commit, don't
> > + # abort if the url cannot be reversed.
> > + return url
> Can you explain this comment a bit more ? Couldn't the caller just catch
> the exception when it is impossible to lookup a bug id from a URL?

This came from my bzr-gtk patch.

I had few errors in this place, they all resulted in a corrupted tree -
as this happens during the uncommit - and, as far as I remember, I had
to copy the tree from a backup, I was not able to repair it.

So, I changed the code to not raise an exception here, and just go on.
As, indeed, in the worst case the user will see the http://something/123
url in the bugs field on his next commit, and he'll be able to fix it
manually.

That's why uncommit should not be aborted here.
But, of course, I can raise an exception here and catch it in the
caller. Do you want me to do it?

> > +
> > class TrackerRegistry(registry.Registry):
> > """Registry of bug tracker types."""
> >
> > @@ -221,6 +238,11 @@
> > """Return the URL for bug_id."""
> > return self.base_url + bug_id
> >
> > + def reverse_engineer(self, url, branch):
> > + if url.startswith(self.base_url):
> > + return self.abbreviation + ':' + url[len(self.base_url):]
> > + return None
> > +
> reverse_engineer is a bit vague, what about something like lookup_id or
> lookup_id_from_url?

Sure. I've changed to lookup_id_from_url()

> For easier consumption by e.g. bzr-gtk it would be nice to use tuples
> here, so there is no need to parse the string returned by
> reverse_engineer().

On the opposite, in the bzr-gtk I only needed the complete bug id, as a
string. If you look in my patch for bzr-gtk, you'd see that there
get_bug_id() returns a tuple :)

Porting it to the core bzr, I've noticed that a tuple is never needed,
and the caller only needs a complete tracker:id string, and I simplified
the interface to take this into account.

As you like. I can change it back, return a tuple everywhere, and join
it into a string in the bzr-gtk. Should I?

> > tracker_registry.register(
> > 'launchpad', UniqueIntegerBugTracker('lp', 'https://launchpad.net/bugs/'))
> > @@ -318,3 +342,26 @@
> > as part of a commit.
> > """
> > return '\n'.join(('%s %s' % (url, FIXED)) for url in bug_urls)
> > +
> > +
> > +def encode_fixes_bug_ids(fixes, branch):
> > + def _iter_bug_fix_urls(fixes, branch):
> > + for fixed_bug in fixes:
> > + tokens = fixed_bug.split(':')
> > + if len(tokens) != 2:
> > + raise errors.BzrCommandError(
> > + "Invalid bug %s. Must be in the form of 'tracker:id'. "
> > + "See \"bzr help bugs\" for more information on this "
> > + "feature.\nCommit refused." % fixed_bug)
> > + tag, bug_id = tokens
> > + try:
> > + yield get_bug_url(tag, branch, bug_id)
> > + except errors.UnknownBugTrackerAbbreviation:
> > + raise errors.BzrCommandError(
> > + 'Unrecognized bug %s. Commit refused.' % fixed_bug)
> > + except errors.MalformedBugIdentifier, e:
> > + raise errors.BzrCommandError(
> > + "%s\nCommit refused." % (str(e),))
> > +
> > + return encode_fixes_bug_urls(_iter_bug_fix_urls(fixes, branch))
> > +
> I'm not sure this is useful refactoring.
>
> Given the way this is being used from bzr-gtk I think it probably makes
> sense to pass in the fixes as a list of tuples rather than unparsed
> strings that are only relevant on the command line.

In my patch I've had this very function verbatim in the bzr-gtk.
That is, bzr-gtk used strings, not tuples.

I can change it to take a list of tuples, but then both commit and
gcommit will need to split strings into tuples before calling this
function.

> BzrCommandError is mainly intended to be used in the command line client
> (e.g. bzrlib/builtins.py) and probably not as useful in an API as the
> UnknownBugTrackerAbbreviaton and MalformedBugIdentifier exceptions.

Sure. I can remove try/except, and put it back in the caller.
But then in the UnknownBugTrackerAbbreviation, I could only say
"Unrecognized bug tracker: %s. Commit refused.", I won't have access to
the bug id there. Is that ok?

Regards,
Sergei