[MERGE] Deprecate export-related InventoryEntry methods

[Martin Pool]

On Mon, Jul 28, 2008 at 12:58 PM, Andrew Bennetts <andrew at canonical.com> wrote:
> 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! :)

It seems like a case of double-dispatch based on both the entry kind
and the destination format.  This is much more closely coupled to the
exporter than to the inventory kind, and it's probably more likely
people will want to add new exporters, so it's probably happier there.

-- 
Martin <http://launchpad.net/~mbp/>