[MERGE] Deprecate export-related InventoryEntry methods

Andrew Bennetts andrew at canonical.com
Mon Jul 28 03:58:50 BST 2008


Andrew Bennetts has voted tweak.
Status is now: Conditionally approved
Comment:
I'm ok with this, I think.

It seems like a small shame to trade several small methods a function 
with large switch statement, though.  Perhaps we should refactor again 
so that we have something like:

----
def _dir_export_directory(fullpath, tree):
     os.mkdir(fullpath)

def _dir_export_file(fullpath, tree):
     osutils.pumpfile(...
     # etc...

dir_exporters.register(kind='directory', func=_dir_export_directory)
dir_exporters.register(kind='file', func=_dir_export_file)
# etc...
----

That way we could fairly easily have unittests that call 
dir_exporters['file'] etc directly.  At the moment we can't unittest 
these pieces easily, because the only way we can invoke this logic is 
from the top-level tar/tgz/dir_exporter functions.  For instance with 
your refactoring a test that exporting to a dir will set or not set the 
execute bit as appropriate would have to do a disproportionate amount of 
work.

It's possible that a registry like that might be useful to plugins if a 
plugin every wants to add a new kind of inventory entry, but that's 
probably only a hypothetical situation.

I don't want to get in the way of landing improvements, so I'm voting 
"tweak": feel free to merge after considering my comments about tests.

If you feel inspired to add a test_export module, that would be perfect! 
:)

For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C48893FE7.9040501%40canonical.com%3E



More information about the bazaar mailing list