[MERGE] Initial support for marking bugs as fixed using bzr
John Arbash Meinel
john at arbash-meinel.com
Mon Apr 16 16:18:07 BST 2007
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Jonathan Lange wrote:
> On 4/13/07, Jonathan Lange <jml at mumak.net> wrote:
>> On 4/13/07, James Westby <jw+debian at jameswestby.net> wrote:
>> > On (11/04/07 14:43), 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.
>> > >
>
> Take #2.
>
> This incorporates James Westby's refactorings, b.d.o support, John's
> review comments (as noted in my reply) and adds reset support to
> ListOption.
>
> Submitting now for your reviewing pleasure.
>
> cheers,
> jml
1) I think it would be good to get this in for 0.16. I think we can
merge the general feature, and then tweak the specifics during feature
freeze. But that is up to Martin. I want to make sure, though, that we
are happy with all of the serialization, because this data is going to
be committed for the long-term. (At the very least, we'll need a way to
"upgrade" it.)
2) Your current serialization format is:
bugs: http://url/1234 fixed,http://url/3456 fixed
A few things...
a) I would at least prefer ", " as the separator, to plain ','.
b) It assumes neither space nor comma appear in urls. comma is a
"reserved" character, (I think it defines a fragment like
semicolon)
I *think* that means we are okay. But what if a tracker wants to
use: "http://myurl/bugs,12345". If that is a stupid thing to do
(from the URL definition perspective) then I won't worry about it.
But if you do (a) above, then the parser splits on ", ". Which is
much less likely to appear by chance. Especially since spaces
should be encoded for urls. (%20)
3) I had some comments about the possible BugTracker api. What you have
is probably fine, but if we want to change it, we should do so before
0.16 is released.
4) Pretty much all of my other comments are "trivial". Like "add a
docstring here". (Most important one being a docstring for ListOption).
...
v- I would still prefer to have class variables:
tag = None
bug_url = None
Technically, "tag" is a class variable, and "bug_url" is an instance
variable. (It may be class, but can be overridden by the instance)
> +
> +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.
> + """
> +
> + @classmethod
> + def get(klass, tag, branch):
> + """Returns the tracker if the tag matches. Returns None otherwise."""
> + if tag != klass.tag:
> + return None
> + return klass()
> +
> + def get_bug_url(self, bug_id):
> + """Return the URL for bug_id."""
> + self.check_bug_id(bug_id)
> + return urlutils.join(self.base_url, bug_id)
> +
> + def check_bug_id(self, bug_id):
> + """Check that the bug_id is valid."""
> + pass
^- Add a single line to the doc string:
The base implementation assumes all bug ids are valid.
> +
> +
> +class UniqueIntegerBugTracker(UniqueBugTracker):
> + """A SimpleBugtracker where the bug ids must be integers"""
> +
> + def check_bug_id(self, bug_id):
> + try:
> + int(bug_id)
> + except ValueError:
> + raise errors.MalformedBugIdentifier(bug_id, "Must be an integer")
> +
v- This seems like it could be implemented in terms of
UniqueIntegerBugTracker. Either by changing "__init__" to:
def __init__(self, url):
self.bug_url = urlutils.join(url, 'ticket')
However, I know we have a general policy of not overloading class
variables with instance variables, because it confuses people who don't
understand all the special cases. (self.lst.append() modifies the class
var unless you do self.lst = [] first, etc)
So one possibility would be to change the base implementation to have an
"__init__()" with something like:
def __init__(self, url):
self._bug_url = url
And the default "get()" function uses:
return klass(klass.bug_url)
And naturally, the "get_bug_url()" should use self._bug_url.
None of this is strictly necessary, what you have written works. I'm
just seeing some redundancy that doesn't seem strictly necessary, and
I'm wondering if a refactoring would make it a nicer api.
We should have settled the API for 0.16 so that we don't have to do
compatibility workarounds in the future. But I think tweaks to the api
could happen after feature freeze.
> +class TracTracker(object):
> + """A Trac instance."""
> +
> + @classmethod
> + def get(klass, tag, branch):
> + """Return a TracTracker for the given tag.
> +
> + Looks in the configuration of 'branch' for a 'trac_<tag>_url' setting,
> + which should refer to the base URL of a project's Trac instance.
> + e.g.
> + trac_twisted_url = http://twistedmatrix.com
> + """
> + url = branch.get_config().get_user_option('trac_%s_url' % (tag,))
> + if url is None:
> + return None
> + return klass(url)
> +
> + def __init__(self, url):
> + self._url = url
> +
> + def get_bug_url(self, bug_id):
> + """Return a URL for a bug on this Trac instance."""
> + try:
> + int(bug_id)
> + except ValueError:
> + raise errors.MalformedBugIdentifier(bug_id, "Must be an integer")
> + return urlutils.join(self._url, 'ticket', bug_id)
> +
> +tracker_registry.register('trac', TracTracker)
>
> === added file bzrlib/tests/test_bugtracker.py // file-id:test_bugtracker.py-20
> ... 070410073305-vu1vu1qosjurg8kb-2
> --- /dev/null
> +++ bzrlib/tests/test_bugtracker.py
> @@ -0,0 +1,182 @@
> +# Copyright (C) 2007 Canonical Ltd
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> +
- -- You should have a doc string
v- And you have 3 spaces after the imports (one too many)
> +
> +from bzrlib import bugtracker
> +from bzrlib.errors import MalformedBugIdentifier
> +from bzrlib.tests import TestCaseWithMemoryTransport
> +
> +
> +
> +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'))
> +
> + 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'))
> +
> + 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')
> +
> +
> +class TestUniqueBugTracker(TestCaseWithMemoryTransport):
> +
> + def test_check_bug_id_passes(self):
> + """check_bug_id should always pass for the base UniqueBugTracker."""
> + tracker = bugtracker.UniqueBugTracker()
> + self.assertEqual(None, tracker.check_bug_id('12'))
> + self.assertEqual(None, tracker.check_bug_id('orange'))
> +
> + def test_joins_id_to_base_url(self):
> + """The URL of a bug is the base URL joined to the identifier."""
> + tracker = bugtracker.UniqueBugTracker()
> + tracker.base_url = 'http://bugs.com'
> + self.assertEqual('http://bugs.com/1234', tracker.get_bug_url('1234'))
^- It would probably be good to test "orange" here, too.
> +
> + def test_returns_tracker_if_tag_matches(self):
> + """The get() classmethod should return an instance of the tracker if
> + the given tag matches the tracker's tag.
> + """
> + class SomeTracker(bugtracker.UniqueBugTracker):
> + tag = 'xxx'
> + base_url = 'http://bugs.com'
> +
> + branch = self.make_branch('some_branch')
> + tracker = SomeTracker.get('xxx', branch)
> + self.assertEqual('xxx', tracker.tag)
> + self.assertEqual('http://bugs.com/1234', tracker.get_bug_url('1234'))
> +
^- Shouldn't we also test what it returns if it doesn't match? (So we
know whether it raises an exception, or returns None, etc).
...
v- Space and docstring
> +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'))
> +
> + def test_get_with_unsupported_tag(self):
> + """If asked for an unrecognized or unconfigured tag, return None."""
> + branch = self.make_branch('some_branch')
> + self.assertEqual(None, bugtracker.TracTracker.get('lp', branch))
> + self.assertEqual(None, bugtracker.TracTracker.get('twisted', branch))
> +
> + def test_get_with_supported_tag(self):
> + """If asked for a valid tag, return a matching TracTracker instance."""
> + branch = self.make_branch('some_branch')
> + config = branch.get_config()
> + config.set_user_option('trac_twisted_url', self.trac_url)
> + tracker = bugtracker.TracTracker.get('twisted', branch)
> + self.assertEqual(
> + bugtracker.TracTracker(self.trac_url).get_bug_url('1234'),
> + tracker.get_bug_url('1234'))
> +
> + def test_get_bug_url_for_bad_bag(self):
> + """When given a bug identifier that is invalid for Launchpad,
> + get_bug_url should raise an error.
> + """
> + tracker = bugtracker.TracTracker(self.trac_url)
> + self.assertRaises(MalformedBugIdentifier, tracker.get_bug_url, 'bad')
>
...
v- As a general rule, we prefer (in a lazy_import section):
from bzrlib import bugtracker
And then using
bugtracker.get_bug_url()
Rather than directly importing functions. There are a few reasons, which
I thought would have been defined in HACKING, but don't seem to be. (The
lazy_import reasons are there)
But generally we want imports at the top for import clarity, and
lazy_imports for startup speed. And 'module.function()' allows
function() to be changed by plugins (monkey patched) without having to
track down other locations.
> + def _get_bug_fix_properties(self, fixes, branch):
> + from bzrlib.bugtracker import get_bug_url
> +
> + properties = []
> + # Configure the properties for bug fixing attributes.
> + 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 'tag:id'. "
> + "Commit refused." % fixed_bug)
> + tag, bug_id = tokens
> + try:
> + bug_url = get_bug_url(tag, branch, bug_id)
> + except KeyError:
> + raise errors.BzrCommandError(
> + 'Unrecognized bug %s. Commit refused.' % fixed_bug)
> + except errors.MalformedBugIdentifier:
> + raise errors.BzrCommandError(
> + "Invalid bug identifier for %s. Commit refused."
> + % fixed_bug)
> + properties.append('%s fixed' % bug_url)
> + return ','.join(properties)
> +
> def run(self, message=None, file=None, verbose=True, selected_list=None,
> - unchanged=False, strict=False, local=False):
> + unchanged=False, strict=False, local=False, fixes=None):
> from bzrlib.commit import (NullCommitReporter, ReportCommitToLog)
> from bzrlib.errors import (PointlessCommit, ConflictsInTree,
> StrictCommitFailed)
v- This is what looked like it was a class variable, but I guess it is
just inside the function. (Not enough context for me, but that is just
diff, not your fault)
> @@ -2096,6 +2122,9 @@
>
> # TODO: do more checks that the commit will succeed before
> # spending the user's valuable time typing a commit message.
> +
> + properties = {}
> +
> tree, selected_list = tree_files(selected_list)
> if selected_list == ['']:
> # workaround - commit of root of tree should be exactly the same
> @@ -2103,6 +2132,8 @@
> # selected-file merge commit is not done yet
> selected_list = []
>
> + properties['bugs'] = self._get_bug_fix_properties(fixes, tree.branch)
> +
> if local and not tree.branch.get_bound_location():
> raise errors.LocalRequiresBoundBranch()
>
...
v- Space and docstring... Especially needs a doc string for how to use it.
"""Add a command line argument which can be repeated.
Each time the option is supplied, its value will be appended to a list.
If the value is '-' the list will be reset.
"""
> +class ListOption(Option):
> + def add_option(self, parser, short_name):
> + """Add this option to an Optparse parser."""
> + option_strings = ['--%s' % self.name]
> + if short_name is not None:
> + option_strings.append('-%s' % short_name)
> + parser.add_option(action='callback',
> + callback=self._optparse_callback,
> + type='string', metavar=self.argname.upper(),
> + help=self.help, default=[],
> + *option_strings)
> +
> + def _optparse_callback(self, option, opt, value, parser):
> + values = getattr(parser.values, self.name)
> + if value == '-':
> + del values[:]
> + else:
> + values.append(self.type(value))
> +
> +
> class RegistryOption(Option):
> """Option based on a registry
>
v- You really shouldn't be using "run_bzr('init')".
We have a policy of using the bzrlib api to set up the tests, and then
only using "run_bzr()" for testing the actual command-line under test.
So it should be:
tree = self.make_branch_and_tree('.')
self.build_tree(['hello.txt'])
tree.add('hello.txt')
out, err = self.run_bzr(
...
> + def test_fixes_bug_output(self):
> + """commit --fixes=lp:23452 succeeds without output."""
> + self.run_bzr("init")
> + self.build_tree(['hello.txt'])
> + self.run_bzr('add', 'hello.txt')
> + output, err = self.run_bzr(
> + 'commit', '-m', 'hello', '--fixes=lp:23452', 'hello.txt')
> + self.assertEqual('', output)
> + self.assertEqual('added hello.txt\nCommitted revision 1.\n', err)
> +
v- You do it correctly here.
> + def test_fixes_bug_sets_property(self):
> + """commit --fixes=lp:234 sets the lp:234 revprop to 'fixed'."""
> + tree = self.make_branch_and_tree('tree')
> + self.build_tree(['tree/hello.txt'])
> + tree.add('hello.txt')
> + self.run_bzr(
> + 'commit', '-m', 'hello', '--fixes=lp:234', 'tree/hello.txt')
> +
> + # Get the revision properties, ignoring the branch-nick property, which
> + # we don't care about for this test.
> + last_rev = tree.branch.repository.get_revision(tree.last_revision())
> + properties = dict(last_rev.properties)
> + del properties['branch-nick']
> +
> + self.assertEqual({'bugs': 'https://launchpad.net/bugs/234 fixed'},
> + properties)
> +
> + def test_fixes_multiple_bugs_sets_properties(self):
> + """--fixes can be used more than once to show that bugs are fixed."""
> + tree = self.make_branch_and_tree('tree')
> + self.build_tree(['tree/hello.txt'])
> + tree.add('hello.txt')
> + self.run_bzr(
> + 'commit', '-m', 'hello', '--fixes=lp:123', '--fixes=lp:235',
> + 'tree/hello.txt')
> +
> + # Get the revision properties, ignoring the branch-nick property, which
> + # we don't care about for this test.
> + last_rev = tree.branch.repository.get_revision(tree.last_revision())
> + properties = dict(last_rev.properties)
> + del properties['branch-nick']
> +
> + self.assertEqual(
> + {'bugs': 'https://launchpad.net/bugs/123 fixed,'
> + 'https://launchpad.net/bugs/235 fixed'},
> + properties)
...
v- Outside the scope here, we need some spacing cleanups (2 blank lines
between top-level entries), and we should just delete the commented out
section. I know this isn't your code, but I went and checked while we
are in the area.
> === modified file bzrlib/tests/test_options.py // last-changed:jml at canonical.co
> ... m-20070413035248-l5lqpwr1t0yzp872
> --- bzrlib/tests/test_options.py
> +++ bzrlib/tests/test_options.py
> @@ -38,16 +38,16 @@
> """Option parser"""
> eq = self.assertEquals
> eq(parse_args(cmd_commit(), ['--help']),
> - ([], {'help': True}))
> + ([], {'fixes': [], 'help': True}))
> eq(parse_args(cmd_commit(), ['--message=biter']),
> - ([], {'message': 'biter'}))
> + ([], {'fixes': [], 'message': 'biter'}))
> ## eq(parse_args(cmd_log(), '-r 500'.split()),
> ## ([], {'revision': RevisionSpec_int(500)}))
>
...
v- Same thing here, we generally don't leave "commented out code".
Unless there is a specific reason.
> @@ -239,3 +240,28 @@
> # (['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 TestListOptions(TestCase):
> + """Tests for ListOption, used to specify lists on the command-line."""
> +
> + def parse(self, options, args):
> + parser = option.get_optparser(dict((o.name, o) for o in options))
> + return parser.parse_args(args)
> +
> + def test_list_option(self):
> + options = [option.ListOption('hello', type=str)]
> + opts, args = self.parse(options, ['--hello=world', '--hello=sailor'])
> + self.assertEqual(['world', 'sailor'], opts.hello)
> +
> + def test_list_option_no_arguments(self):
> + options = [option.ListOption('hello', type=str)]
> + opts, args = self.parse(options, [])
> + self.assertEqual([], opts.hello)
> +
> + def test_list_option_can_be_reset(self):
> + """Passing an option of '-' to a list option should reset the list."""
> + options = [option.ListOption('hello', type=str)]
> + opts, args = self.parse(
> + options, ['--hello=a', '--hello=b', '--hello=-', '--hello=c'])
> + self.assertEqual(['c'], opts.hello)
>
^- You should have a test that you can set "type=int", and have the list
filled with integers. Since "type=" is part of the api.
Overall, good work. I feel like I'm being picky, but if we clean up the
small things, we only have to do it once, rather than doing full code
reviews.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iD8DBQFGI5OvJdeBCYSNAAMRAnKUAJ92MAAvNiHi8UU8btlaEeYM/39dfwCgycXO
jgTlmuDHmev8Ps4Pi8q3gqI=
=uDRh
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list