Code review comment for lp:~spiv/bzr/merge-into-merger

Revision history for this message
Robert Collins (lifeless) wrote :

id2path('random') -> None
So I think that that is fine.

I'm still ambivalent about avoiding inventory - inventory is the /model/ for the tree, after all. Anyhow, looks doable here.

_finish_transform doesn't pair well with compute_transform. Perhaps finish_computing_transform?

The new exception should be in errors.py until we change our policy on that.

Rather than _Wrapper I suggest MergeTypeParameteriser. Or something like that. Long but less 'zomg I need to read the code' than Wrapper ;)

I think your tests really belong in the main merge test script, merge_into was at most a plugin name, and this is core code - its tests should be matching the module the code is in as per our convention for finding tests.

What do you think of making an existing-but-empty dir be replaced rather than conflicted ?

review: Needs Fixing

« Back to merge proposal