Code review comment for lp:~vila/bzr/conflict-manager

101 +class ResolveActionOption(option.RegistryOption):
102 +
103 + def __init__(self):
104 + super(ResolveActionOption, self).__init__(
105 + 'action', 'How to resolve the conflict.',
106 + value_switches=True,
107 + registry=resolve_action_registry)
108 +
109 +

^- This only seems useful if you were actually going to re-use the
ResolveActionOption.
I realize you have some tests for it, but it doesn't do anything beyond a
standard RegistryOption.

Were you thinking to have the '--auto' logic as part of the option object?

168 +def resolve(tree, paths=None, ignore_misses=False, recursive=False,
169 + action='done'):
^- Having a top-level function in builtins.py to do command specific actions
isn't particularly nice, though I realize this wasn't from you. Perhaps just a
comment that it should be moved into a bzrlib.resolve* module?

236 + def _do(self, action, tree):
237 + """Apply the specified action to the conflict.
238 +
239 + :param action: The method name to call.
240 +
241 + :param tree: The tree passed as a parameter to the method.
242 + """
243 + meth = getattr(self, action, None)
244 + if meth is None:
245 + raise NotImplementedError(self.__class__.__name__ + '.' + action)
246 + meth(tree)

^- To avoid namespace collisions, this would probably be better as:

meth = getattr(self, 'action_' + action, None)
...

=== modified file 'bzrlib/tests/test_conflicts.py'
971 --- bzrlib/tests/test_conflicts.py 2010-01-04 14:56:59 +0000
972 +++ bzrlib/tests/test_conflicts.py 2010-02-04 13:06:32 +0000
973 @@ -21,8 +21,10 @@
974 bzrdir,
975 conflicts,
976 errors,
977 + option,
978 tests,
979 )
980 +from bzrlib.tests import script

^- script tests don't seem to be a great match for whitebox testing. And
looking at the scripts themselves they certainly look like blackbox tests.
(using commit to assert that things are ok.) Versus even an "assert*" function.

The tests seem fine, but they look like they should belong in a
blackbox/test_resolve.py module.