Code review comment for lp:~james-w/linaro-image-tools/tarfile-improvements

On Tue, 31 Aug 2010 01:13:40 -0000, Michael Hudson <email address hidden> wrote:
> Review: Approve
> hwpack/better_tarfile.py could use a module docstring.

Done, thanks.

> It's not clear to me why standard_tarfile or better_tarfile have
> default arguments: they're both only ever called with the
> non-defaulted arguments supplied (relatedly, I wonder what the
> combination of mode and fileobj to TarFile.__init__ means.....)

Later branches supply different mode arguments, so I probably should
have made that change there.

mode can also be used to specify the compression that will be used, so
while a mode of "w" on a read-only file object makes no sense, it is
needed to have both.

> The standard_tarfile context manager is only used in tests, so maybe
> it should be defined in the test module.

It's used in two test modules later, which is why I moved it here, but
I'll move it to a testing module (if it stays given your comments on the
next proposal.)

> I don't like the way that the tests "know" that create_simple_tarball
> creates a file called foo with content bar. I think it would be
> better to use a pattern like bzrlib's build_tree where you say
> create_simple_tarball([('foo', 'bar')]) or something.

I think that's a good idea. I had them all explicitly creating the
paths, but given that they all created identical paths I just factored
it out.

The other option would be fixture, where the path would be an attribute,
and so wouldn't be hardcoded like that.

> All else looks ok.

Thanks, tweaks are on their way.

James