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

[Jonathan Lange]

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 = {}
    properties.update(self._get_bug_fix_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
wanted.

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.

Fixed.

> 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"]
>

Fixed.

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

Thanks for the helpful review.

cheers,
jml