[MERGE] Initial support for marking bugs as fixed using bzr

[John Arbash Meinel]

I tried to submit this through BundleBuggy, but it gave me a 500
Internal Error.

So here it is in an email instead.

Jonathan Lange wrote:
> Adds a '--fixes' option to commit to allow users to specify that the
> commit fixes certain bugs. Bugfix information is stored as <bug_url>:
> <status> entry in the properties dict.
> 
> Based on discussions w/ lifeless and jamesh at the recent Sydney
> mini-sprint.
> 
> cheers,
> jml

Overall, it looks quite good. Just a few things to mention.

1) NEWS entry. Unless I'm mistaken this closes a few bugs, they should
be updated in the tracker, and we should add NEWS entries about them.

2) This is something that should really have some documentation about it
in doc/*.txt
   It is a nice feature, and it would be good to make it easy for people
to use.

3) Should the 'LaunchpadTracker' implementation be built-in, or part of
the Launchpad plugin. Since we have an lp plugin, I would think to put
it there. (Remember that plugins can also define test suites, so it
still should be tested)

4) I'm not sure if we want Command classes to have a lot of extra
properties and functions. At least we haven't typically done it in that
fashion. I think partially because Command is just for doing glue
between command line and internals, and it is easier to directly test
internals if they aren't a Command object.

My biggest complaint is just that you have:

class cmd_commit(...):
  properties = {}

 def run():
   self.properties.update()

Which means that you are modifying a class variable based on an
instance. Which would have really bad side-effects for something like a
long-running gui. (Olive). Because now you start building up state after
each commit. (Now I would actually expect Olive to not use cmd_commit to
do its commits, but you get my idea).

5) It doesn't seem like the fallback bug tracker should always be a
'trac' bug tracker. What about bugzilla, etc. So it seems a better form
might be to have a config for:

twisted_bug_tracker_style = trac
trac_twisted_bug_url = http://twistedmatrix/trac

So when we find a prefix we don't instantly recognize, we look for
%(prefix)_bug_tracker_style
and then instantiate
tracker = tracker_registry.get(style)
tracker.get_bug_url(prefix, data)

Either that, or I would do:

bzr bug --fixes trac:twisted:1004

leaving you with
bzr bug --fixes bugzilla:mozilla:1234

I prefer the former, because then you can associate a project (short
name) with a bug tracker, even if it takes a bit more internal
configuration.

I suppose you already have that behavior by having:

trac_twisted_url = XXX
bugzilla_mozilla_url = YYY

I suppose people are just stupid if they do:
trac_upstream_url = XXX
bugzilla_upstream_url = YYY

since they could do:
trac_trac_upstream_url = XXX
bugzilla_bugzilla_upstream_url = YYY

having

bzr commit --fixes upstream:XXX

seems nice.



6) And now a few code-level comments:
+from bzrlib.lazy_import import lazy_import
+lazy_import(globals(), """
+from bzrlib import errors, registry
+""")

No need to lazily import 'registry' when the first thing you do is inherit:

class TrackerRegistry(registry.Registry):

Non lazy imports also work a little better with things like pydoctor. So
I'd like to do them when it matters, but not when they are instantly
triggered.


7) I prefer 'self.run_bzr' to 'self.capture'.
+        output = self.capture('commit -m hello --fixes=lp:23452 hello.txt')
+        self.assertEqual('', output)

output, err = self.run_bzr('commit', '-m', 'hello', '--fixes=lp:23452',
'hello.txt')

Just because you don't have to worry about potential whitespace issues.
Oh, and it gives you a chance to check
self.assertEqual('', err)

to make sure there aren't warnings being written to stderr.

I would also avoid 'self.runbzr' since it isn't as complete as run_bzr.
+        self.runbzr(
+            'commit -m hello --fixes=lp:123 --fixes=lp:235 tree/hello.txt')




8) I usually write:
+        """If the given tag is unrecognized, return None."""
+        self.assertEqual(
+            None, bugtracker.LaunchpadTracker.get('twisted', self.branch))

as
+        """If the given tag is unrecognized, return None."""
+        self.assertEqual(None,
+                 bugtracker.LaunchpadTracker.get('twisted', self.branch))

You might also use "self.assertIs(None," since 'x is None' is preferred
in python to 'x == None'. I don't think it really matters here.


9) I'm a little concerned that the data is stored as "<bug_url>:<status>"
I don't remember any specific restrictions on revision properties, but I
think there are some things like they must be ascii, and shouldn't
contain spaces. Also, it seems like it should be:
bug: fixed <url>

Otherwise if someone else decides to use a URL to indicate something
other than a bug, other processing code needs to say whether it
recognizes this exact url, etc.

10) I think we should avoid the word "malformed" in user visible comments:
"Bug identifier %(bug_id)s is malformed: %(reason)s"

I think some people won't understand what it means. Maybe
"Bug identifier %(bug_id)s is not understood: %(reason)s"
or:
"Bug identifier %(bug_id)s is not properly formed: %(reason)s"
or:
"Did not understand bug identifier %(bug_id)s: %(reason)s"

I know it makes sense to coders what 'formed' means, but I don't think
it is common.

11) Why do you use:
+            ["Invalid bug %s. Must be in the form of 'tag:id'. Commit
refused."
+             % 'orange'],

Rather than just saying:
["Invalid bug orange. Must be in the form..."]

Also, these strings are regexes, so the 'correct' form is:
[r"Invalid bug orange\. Must be in the form"]

12) I would change the name of the class:
+++ bzrlib/tests/test_options.py
@@ -239,3 +239,19 @@
 #     (['log'], {'verbose': True, 'revision': [<RevisionSpec_int 500>,
<RevisionSpec_int 600>]})
 #     >>> parse_args('log -rrevno:500..600'.split()) #the r takes an
argument
 #     (['log'], {'revision': [<RevisionSpec_revno revno:500>,
<RevisionSpec_int 600>]})
+
+
+class ListOptions(TestCase):
+    def parse(self, options, args):
+        parser = option.get_optparser(dict((o.name, o) for o in options))
+        return parser.parse_args(args)

Should be called:
class TestListOptions(TestCase):

And there should be a space between class and the first function. (Which
also encourages you to add a doc string, which also makes documentation
generators happy)

John
=:->

PS> Sorry if things are a little confused. It is a little bit difficult
to do in-depth reviews with Bundle Buggy. Maybe just because the comment
box is only 6 lines tall.