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

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

hwpack/better_tarfile.py could use a module docstring.

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.....)

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

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.

All else looks ok.

review: Approve

« Back to merge proposal