[MERGE] Initial support for marking bugs as fixed using bzr
Robert Collins
robertc at robertcollins.net
Thu Apr 19 05:24:41 BST 2007
On Fri, 2007-04-13 at 17:33 +1000, Jonathan Lange wrote:
--- /dev/null
+++ bzrlib/bugtracker.py
@@ -0,0 +1,148 @@
...
....+
+Thus, this module provides a registry of types of bug tracker (e.g. Launchpad,
+Trac). Given a short-hand tag (e.g. 'lp', 'twisted') and a branch with
+configuration information, these tracker types can return an instance capable
+of converting bug IDs into URLs.
+"""
I think referring to these as 'tags' is potentially confusing. Branch
have tags. Chatting on IRC we came up with
'abbreviated_bugtracker_name'. While this is a mouthful, its clear what
it refers to. I'd like you to look for 'tag' throughout this file and
use a more precise name that isn't overloaded within bzrlib already.
E.g. withing the tracker classes, perhaps 'abbreviation' is a good
attribute name.
+
+def get_bug_url(tag, branch, bug_id):
+ """Return a URL pointing to the bug identified by 'bug_id'."""
+ return tracker_registry.get_tracker(tag, branch).get_bug_url(bug_id)
'pointing to the bug' is a little misleading - surely its pointing to
the webpage for the bug. (That is, we're not using URN style
identification of the bug).
...
+ for tracker_name, tracker_type in self.iteritems():
+ tracker = tracker_type.get(tag, branch)
+ if tracker is not None:
+ return tracker
+ raise KeyError("No tracker found for %r on %r" % (tag, branch))
tracker_name is not used here. Why not use self.values() ? (Or isn't that defined).
+
+tracker_registry = TrackerRegistry()
+"""Registry of bug trackers."""
Perhaps this docstring is better served by having a __doc__ on instances
of TrackerRegistry - wouldn't that work for pydoc?
+
+class UniqueBugTracker(object):
+ """A style of bug tracker that exists in one place only, such as Launchpad.
+
+ If you have one of these trackers then subclass this and add attributes
+ named 'tag' and 'base_url'. The former is the tag that the user will use
+ on the command line. The latter is the url that the bug ids will be
+ appended to.
+
+ If the bug_id must have a special form then override check_bug_id and
+ raise an exception if the bug_id is not valid.
+ """
As a side note, launchpad doesn't strictly exist in only one place - the
demo and staging servers have different databases to the production
server and can thus talk about different bug numbers. I'm not sure it
matters - but perhaps for testing it would be nice to be able to have
throwaway branches that you use with e.g. staging, and have the right
urls created.
+...
I like that you supply trackers for Launchpad, Debian, and Trac.
Bugzilla is another common tracker and its a simple integer tracker but
like trac appears at many urls. I think it would be good for the code
factoring to implement Bugzilla urls too. (giving us two 'singleton'
trackers, and two 'many instance' trackers).
I dont like that you have subclasses for Launchpad and Debian that vary
solely in parameters. That to me cries out for instances. This would in
principal allow configuring abbreviations for any tracker that we have
the basic facility for via branch.conf - or any other config file in the
configuration system.
I am not making configuration via branch.conf a requirement for getting
+1 - a bug report is fine. But I do think getting rid of the spurious
classes is important and want that changed to get +1.
+++ bzrlib/tests/test_bugtracker.py
@@ -0,0 +1,182 @@
+class TestGetBugURL(TestCaseWithMemoryTransport):
+ """Tests for bugtracker.get_bug_url"""
+
+ def test_get_launchpad_url(self):
+ """No matter the branch, lp:1234 should map to a Launchpad URL."""
+ branch = self.make_branch('some_branch')
+ self.assertEqual(
+ 'https://launchpad.net/bugs/1234',
+ bugtracker.get_bug_url('lp', branch, '1234'))
This test claims to test a negative, which isn't really possible. I
think its more accurate to test something like 'The launchpad tracker
does not consult the branch for determining its urls', and test that
passing in 'None' as the branch results in the right URL.
+ def test_get_trac_url(self):
+ trac_url = 'http://twistedmatrix.com/trac'
+ branch = self.make_branch('some_branch')
+ config = branch.get_config()
+ config.set_user_option('trac_twisted_url', trac_url)
+ self.assertEqual('%s/ticket/1234' % trac_url,
+ bugtracker.get_bug_url('twisted', branch, '1234'))
This tests that one branch has the right url, but you need two to tell
that the parameterisation works. E.g. change the config option and test
the result changes.
+ def test_unrecognized_tag(self):
+ """If the tag is unrecognized, then raise a KeyError."""
+ branch = self.make_branch('some_branch')
+ self.assertRaises(KeyError,
+ bugtracker.get_bug_url, 'xxx', branch, '1234')
I think we should have a better error than KeyError - one that has a good format string for the UI.
...
+class TestUniqueIntegerBugTracker(TestCaseWithMemoryTransport):
+
+ def test_check_bug_id_only_accepts_integers(self):
+ """An UniqueIntegerBugTracker only accepts integers as bug IDs."""
+ tracker = bugtracker.UniqueIntegerBugTracker()
+ self.assertEqual(None, tracker.check_bug_id('1234'))
Is the return code relevant? I would have written this as two separate tests:
one that does
--
tracker.check_bug_id('1234')
--
and one that does
+ self.assertRaises(MalformedBugIdentifier, tracker.check_bug_id, 'red')
+class TestLaunchpadTracker(TestCaseWithMemoryTransport):
I think all these tests can go with the refactoring I proposed earlier -
all you need is a test that the launchpad_tracker is a registered
instance with the right parameters.
+class TestDebianTracker(TestCaseWithMemoryTransport):
Likewise.
+class TestTracTracker(TestCaseWithMemoryTransport):
+ def setUp(self):
+ TestCaseWithMemoryTransport.setUp(self)
+ self.trac_url = 'http://twistedmatrix.com/trac'
+
+ def test_get_bug_url(self):
+ """A TracTracker should map a Trac bug to a URL for that instance."""
+ tracker = bugtracker.TracTracker(self.trac_url)
+ self.assertEqual(
+ '%s/ticket/1234' % self.trac_url, tracker.get_bug_url('1234'))
This seems to be duplicated with the earlier trac test. I think the
problem is that the first test is not clear about what its testing. Is
it testing the convenience api or the underlying api. I think that as
its claiming to test the convenience API, what you should be doing is
registering transient bug trackers up there, and checking they get
called into. That would reduce test duplication.
--- NEWS
+++ NEWS
@@ -5,6 +5,9 @@
* Merge directives can now be supplied as input to `merge` and `pull`,
like bundles can. (Aaron Bentley)
+ * New option '--fixes' to commit, which stores bug fixing annotations as
+ revision properties. (Jonathan Lange, James Henstridge, Robert Collins)
+
INTERNALS:
* bzrlib API compatability with 0.8 has been dropped, cleaning up some
There should be an entry here talking about the new API as well - as
plugins can now add bug trackers.
--- bzrlib/builtins.py
+++ bzrlib/builtins.py
@@ -2074,6 +2074,8 @@
Option('strict',
help="refuse to commit if there are unknown "
"files in the working tree."),
+ ListOption('fixes', type=str,
+ help="XXX -- fix a bug"),
This needs fixing.
@@ -2083,8 +2085,32 @@
]
aliases = ['ci', 'checkin']
=== modified file bzrlib/errors.py // last-changed:jml at canonical.com-2007041305
... 1007-cq318kt5uj59k1ty
--- bzrlib/errors.py
+++ bzrlib/errors.py
@@ -2046,3 +2046,12 @@
def __init__(self, tag_name):
self.tag_name = tag_name
+
+
+class MalformedBugIdentifier(BzrError):
+
+ _fmt = "Did not understand bug identifier %(bug_id)s: %(reason)s"
+
+ def __init__(self, bug_id, reason):
+ self.bug_id = bug_id
+ self.reason = reason
We have tests for error formatting in bzrlib/test_errors.py - can you add one please.
--- bzrlib/option.py
+++ bzrlib/option.py
@@ -205,6 +205,26 @@
yield self.name, self.short_name(), argname, self.help
+class ListOption(Option):
+ def add_option(self, parser, short_name):
class docstring please, and PEP8 vertical whitespace.
IIRC Aaron had serious concerns about ListOption, have they been resolved?
This also needs documentation in:
doc/configuration.txt (for the trac bug tracker configuration). It might
be nice to expand the tutorial (doc/tutorial.txt) to cover using this
(perhaps using the launchpad tracker in the tutorial, as it does not
need configuration).
I think theres a need for a 'managing a project with bzr' document - not
one about centralised/decentralised etc, but about the features of bzr
that are like this one, useful outside the strict 'I can commit code'
space. Thats something I'll file a bug on though, its not needed for
this to land.
I'd like my points addressed and a re-review subsequently, because I
think the incremental changes will be significant.
Cheers,
Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20070419/ead3bc74/attachment.pgp
More information about the bazaar
mailing list