[MERGE REVIEW] --reprocess support for weaves
aaron.bentley at utoronto.ca
Mon Apr 10 06:43:50 BST 2006
-----BEGIN PGP SIGNED MESSAGE-----
Martin Pool wrote:
+ """Base class for text-mergers
+ Subclasses must implement _merge_struct.
| OK, but what is _merge_struct? Perhaps add a base implementation that
| raises NotImplementedError and has a docstring describing what it
| should return. (A sequence of tuples for conflicted/non- conflicted
Okay, I've added a NotImplemented method for it.
+ Many methods produce or consume structured merge information.
+ This is an iterable of tuples of lists of lines.
+ Each tuple may have a length of 1 - 3, depending on whether the
+ represents is conflicted.
+ Unconflicted region tuples have length 1.
+ Conflicted region tuples have length 2 or 3. Index 1 is text_a,
+ Index 1 is text_b, e.g. OTHER. Index 2 is optional. If
+ represents BASE.
| This could do with more documentation: how do you get one of these
| objects, and what do you do with it when you have it?
It originates in _merge_struct(), is used as input for struct_to_lines,
iter_useful, and reprocess_struct, and is the output of iter_useful,
merge_struct, and reprocess_struct.
Callers will rarely need to call anything other than merge_lines(), but
I didn't see any harm in making the others public.
| The summary should be on the same line as the opening quote.
+ def __init__(self, lines_a, lines_b, a_marker='<<<<<<< \n',
+ b_marker='>>>>>>> \n', split_marker='=======\n'):
+ TextMerge.__init__(self, a_marker, b_marker, split_marker)
| To avoid repetition perhaps the default markers should be constants in
| the base class?
- - " type. %s" % merge_type)
+ " type. %s" % self.merge_type)
| May be better as
| "Reprocess is not supported for merge type %s"
| Perhaps we should switch here to a more concrete name than "reprocess",
| e.g. "conflict reduction".
Hmm. Yes, I like this.
+ """Weave merge that takes a plan as its input.
+ This exists so that VersionedFile.plan_merge is implementable.
+ we'd just use WeaveMerge everywhere.
| I can understand having this class split out but I don't understand
| this comment. Would it be OK to say
| Most callers will want to use WeaveMerge instead.
| Does this need to be a different class? Why not just have a
| WeaveMerge.from_plan static method, and then the regular constructor
| will get the plan and call that.
My reasoning was that a from_plan static method would still need to
produce an instance of WeaveMerge, because it needs to return an object
that supports merge_lines. Since the WeaveMerge constructor requires
data that it doesn't have (weave, version_a, version_b), it wouldn't be
able to create an instance of WeaveMerge.
An alternative would be to allow WeaveMerge.__init__ to accept a plan
instead of the weave, version_a and version_b. I thought it was cleaner
to have two classes.
I've attached the updated patch.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 28521 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060410/3782af89/attachment.bin
More information about the bazaar