[RFC] Inventory refactoring (brisbane)
Ian Clatworthy
ian.clatworthy at internode.on.net
Mon Apr 6 09:03:57 BST 2009
This is the part of brisbane-core that I'm most concerned
about landing. Unlike nearly every other change in that
branch, this one can potentially break existing users on
existing formats and/or break plugins. Here's why ...
bzr.dev has one Inventory class with a bunch of methods.
brisbane-core has 2 Inventory classes: Inventory and
CHKInventory, both based on a new abstract class,
CommonInventory. Many of the methods in the old Inventory
class now live in CommonInventory so they are part of
the CHKInventory API. But a pile of methods aren't:
1. Unlike Inventories, CHKInventories are immutable so
methods like add/remove_recursive_id/rename aren't
permitted by design.
2. Several of the "read" methods on Inventory haven't been
moved up to CommonInventory and haven't been re-implemented
in CHKInventory yet.
Methods in the latter class are (from my notes a few weeks back):
* get_file_kind
* get_child
* get_idpath
* has_filename
Inventory also has custom versions of these and maybe CHKInventory
ought to have them as well?
* __eq__
* __ne__
* __hash__
* __repr__
So I think we need to either add get_file_kind, etc. to CommonInventory
and/or CHKInventory *or* deprecate them. Otherwise, we run the risk of
some code somewhere doing something like
revtree.inventory.xxx()
and xxx() not existing because this is a CHK repo, not a pack one.
So, part of the problem here is that many of the "read" methods
on Inventory do *not* have test coverage yet. We need to add that
and then use test multiplication to ensure they work on both
Inventory and CHKInventory objects.
As this is arguably the critical path for merging brisbane-core
and I'm offline tomorrow for medical reasons, can someone else
please consider this problem, and if they agree with me, write
these missing tests ASAP?
Ian C.
PS: I'm happy with the code that is there and I've spent a lot of
time reviewing and tweaking it in recent weeks. The issue is the
stuff which is *not* there.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bris-inventory.diff
Type: text/x-diff
Size: 76552 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090406/65a965d4/attachment-0001.bin
More information about the bazaar
mailing list