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

Jonathan Lange jml at mumak.net
Fri Apr 13 06:26:50 BST 2007

On 4/12/07, John Arbash Meinel <john at arbash-meinel.com> wrote:
> 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.

I've added a simple news entry for the new option for commit. However,
I couldn't find any bugs that this branch fixes. Got any in mind?

> 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.

Right. I haven't added one yet, but I will once all of the other
changes prompted by this thread are finished.

> 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.

_get_bug_fix_properties is the only function I add. I think it makes
cmd_commit clearer to keep it separate from run(). I'd be happy to
move it if you can recommend a better home for it.

> My biggest complaint is just that you have:
> class cmd_commit(...):
>   properties = {}
>  def run():
>    self.properties.update()

I can't see that in my code. I see:

class cmd_commit(...):
  def _get_bug_fix_properties(...):
    properties = {}
    return properties

  def run(...):
    properties = {}

All the variables are local, not instance or class.

> 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.

Trac isn't the fallback tracker. I just needed to have another simple
non-Launchpad tracker in order to be able to write the sort of tests I

Regarding naming conflicts. The configuration is on the branch. As I
understand Bazaar's config system, this means that you could define
  trac_upstream_url = XXX
on one branch and
  bugzilla_upstream_url = YYY
on another branch, and not worry about naming conflicts.

I'd be happy to switch to an explicitly configured tracker style, as
long as 'lp:' resolves to Launchpad by default.

> 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.

Fixed. 'errors' is still lazily imported -- is this correct?

> 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')

The 'fixes' tests now use run_bzr.

> 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.

I usually use the second form also (afaict with these variable-width
fonts). Changed to use that and assertIs.

> 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.

I see what you mean.

The only documentation I could find on revprop format is that the key
could not contain spaces.

Perhaps "bugs: <bug_url> <status>, ..." would be better. That clearly
groups all the bug info and maps from each bug to its new status.

> 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'],

No idea. Changed.

> 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)

Changed class name. Docstring and newline added.

> 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.

No worries. The main confusion was not always knowing which file had
the code that you were commenting on. Thankfully grep and memory serve

Thanks for the helpful review.


More information about the bazaar mailing list