[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