Code review comment for lp:~doxxx/bzr/mergetools

I think the idea of having a common mergetools set is a good one. And is
something that we want to bring into core. I think this still needs some
updates, but it is getting close to a point where I would be happy to merge it.

18 + if merge_tool is None:
19 + available = '\n '.join([mt.get_name() for mt in
20 + mergetools.get_merge_tools()
21 + if mt.is_available()])
22 + raise errors.BzrCommandError('Unrecognized merge tool: %s\n\n'
23 + 'Available merge tools:\n'
24 + ' %s' % (tool_name, available))

^- This specific code seems to be something that should live in mergetools itself. I generally don't like top-level functions in 'builtins.py'. Maybe just move the whole thing to mergetools and use "mergetools.resolve_using_mergetool()" inside the command code?

Importing "mergetools" should also be done in the "lazy_import" section. which you may have already done.

63 + retval = self._do_merge(merger, change_reporter, allow_pending,
64 + verified)
65 + if retval != 0 and resolve_using is not None:
66 + _resolve_using_merge_tool(resolve_using, tree.conflicts())
67 + return retval

^- The spacing is odd here (yes, I realize Copy & Paste messed it up). But what I see is about 8 character indentation, when I would expect 4. Maybe you have a tab char? (sources with tab characters will be rejected by the test suite.)

104 ('cmd_resolve', ['resolved'], 'bzrlib.conflicts'),
105 ('cmd_conflicts', [], 'bzrlib.conflicts'),
106 ('cmd_sign_my_commits', [], 'bzrlib.sign_my_commits'),
107 +<<<<<<< TREE
108 ('cmd_test_script', [], 'bzrlib.cmd_test_script'),
109 +=======
110 + ('cmd_test_script', [], 'bzrlib.tests.script'),
111 + ('cmd_mergetools', [], 'bzrlib.mergetools'),
112 +>>>>>>> MERGE-SOURCE

^- most lists like this we try to keep in sorted order. I realize this one isn't, but it probably should be. (That way, most inserts go into a different location, rather than all-at-the-end and it helps avoid spurious conflicts like this.)

There is duplicate code between "MergeTool.__init__" and "MergeTool.set_commandline". In general, it seems like there are a lot of functions in there that we won't actually want to use. So potentially paring it down a bit would be a good thing. However, if we do want it, then __init__ should probably just call self.set_commandline() rather than copying the code locally.

I agree that "cmd_mergetools" seems a bit redundant with "cmd_config", but it also has stuff like --detect, which means it can do more than config would.

It is probably fine to leave it as is.

716 === added file 'bzrlib/tests/test_mergetools.py'
717 --- bzrlib/tests/test_mergetools.py 1970-01-01 00:00:00 +0000
718 +++ bzrlib/tests/test_mergetools.py 2010-10-30 22:41:24 +0000
719 @@ -0,0 +1,242 @@
720 +# -*- coding: utf-8 -*-
We don't use coding statements. if you need non-ascii characters, use their escaped form. (u'\xb5', etc.)

The tests are a bit clustered (and some lines are longer than 80 chars). Rather than doing:

749 +class TestMergeTool(tests.TestCaseInTempDir):
750 + def test_basics(self):
751 + mt = mergetools.MergeTool('tool', '/path/to/tool --opt %b -x %t %o --stuff %r')
752 + self.assertEquals('/path/to/tool --opt %b -x %t %o --stuff %r', mt.get_commandline())
753 + self.assertEquals(['/path/to/tool', '--opt', '%b', '-x', '%t', '%o',
754 + '--stuff', '%r'], mt.get_commandline_as_list())
755 + self.assertEquals('/path/to/tool', mt.get_executable())
756 + self.assertEquals('--opt %b -x %t %o --stuff %r', mt.get_arguments())
757 + self.assertEquals('tool', mt.get_name())
758 + mt.set_name('bettertool')
759 + mt.set_commandline('/new/path/to/bettertool %b %t %o %r')
760 + self.assertEquals('/new/path/to/bettertool %b %t %o %r', mt.get_commandline())
761 + self.assertEquals(['/new/path/to/bettertool', '%b', '%t', '%o', '%r'],
762 + mt.get_commandline_as_list())
763 + self.assertEquals('/new/path/to/bettertool', mt.get_executable())
764 + self.assertEquals('%b %t %o %r', mt.get_arguments())
765 + self.assertEquals('bettertool', mt.get_name())
766 + mt.set_executable('othertool')
767 + self.assertEquals('othertool', mt.get_executable())
768 + self.assertEquals('othertool %b %t %o %r', mt.get_commandline())
769 + self.assertEquals(['othertool', '%b', '%t', '%o', '%r'],
770 + mt.get_commandline_as_list())
771 + mt.set_arguments('%r %b %t %o')
772 + self.assertEquals('%r %b %t %o', mt.get_arguments())
773 + self.assertEquals('othertool %r %b %t %o', mt.get_commandline())
774 + self.assertEquals(['othertool', '%r', '%b', '%t', '%o'],
775 + mt.get_commandline_as_list())
776 + mt = mergetools.MergeTool(None, '/path/to/tool blah stuff etc')
777 + self.assertEquals('tool', mt.get_name())
778 +

You could do:

class TestMergeTool(tests.TestCase):

    def setUp(self):
        super(TestMergeTool, self).setUp()
        self.tool = mergetools.MergeTool('tool',
            '/path/to/tool --opt %b -x %t %o --stuff %r')

    def test_get_commandline(self):
        self.assertEqual('/path/to/tool --opt %b -x %t %o --stuff %r',
                         self.tool.get_commandline())

    def test_get_commandline_as_list(self):
        self.assertEqual(['/path/to/tool', '--opt', '%b', '-x', '%t', '%o',
                          '--stuff', '%r'],
                         self.tool.get_commandline_as_list())

etc.

The main reason is because it prevents accidental chaining. (changing one bit
of the test shouldn't cause the rest of the test to fail, etc.)

When possible, a given "test_*" should test a single aspect, and only that.

I also prefer "assertEqual()" but that is just a personal preference. You don't
need to change it for me. I *would* like to see the tests split up a bit more.

As part of the "-*- coding" stuff, I wouldn't use long words for the unicode
tests:

779 + def test_unicode(self):
780 + mt = mergetools.MergeTool(u'Инструмент', u'/path/to/Инструмент --opt %b -x %t %o --stuff %r')

Simply doing:

mt = mergetools.MergeTool(u'b\xb5r')
or something along those lines.

It will make it easier for those of us that don't understand the language to
maintain the tests.