Bazaar

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

conflict-manager
Merge into bzr.dev

Revision history for this message

Vincent Ladeuil (vila) wrote on 2010-02-05:

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

You're right, I introduced it for tests only.
The alternative was to define it in tests only too which was more likely to break in the future.

I can reconsider later if you feel strongly about it but overall I think it makes things simpler.

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

Hopefully not :)

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

Meh, that's in conflicts.py and I don't think creating a resolve.py module is right. resolve is an action on conflict objects.

It *could* arguably become part of the MutableTree API and get a @needs_tree_write_lock
decorator or become a method of ConflictList.

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

Good point. Fixed.

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

Right, they served as a test bed for shell-like tests and like most shell-like tests they should be rewritten :) I'll add a TODO for that and will address it shortly.

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

You're right, I introduced it for tests only.
The alternative was to define it in tests only too which was more likely to break in the future.

I can reconsider later if you feel strongly about it but overall I think it makes things simpler.

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

Hopefully not :)

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

Meh, that's in conflicts.py and I don't think creating a resolve.py module is right. resolve is an action on conflict objects.

It *could* arguably become part of the MutableTree API and get a @needs_tree_write_lock
decorator or become a method of ConflictList.

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

Good point. Fixed.

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

Right, they served as a test bed for shell-like tests and like most shell-like tests they should be rewritten :) I'll add a TODO for that and will address it shortly.

« Back to merge proposal