ascoderu / xtarfile

Wrapper around tarfile to add support for more compression formats
Apache License 2.0
8 stars 5 forks source link

Complete rewrite using TarFile as baseclass #10

Closed meganomic closed 3 years ago

meganomic commented 3 years ago

I rewrote the entire thing using TarFile as a baseclass because I think it's prettier. It uses TarFiles inbuilt OPEN_METH variable to register new compression formats. I'm 99% sure it doesn't have any breaking changes.

I'm suppressing some Flake8 errors that I think are silly. You might think otherwise. Is an easy fix if you do.

Had to rewrite the tests so while I was at it I used pytest instead of the deprecated setup.py test.

I also rewrote the setuptools code to use the new setup.cfg style.

There is also a tox.ini file for Tox support. I don't think it's very useful though. Should probably just get rid of it.

Any ideas or comments?

meganomic commented 3 years ago

I don't know how to fix the ci.yml file to reflect the changes. I tried my best.

meganomic commented 3 years ago

Okay, I think I finally fixed all the CI issues. I just needed to sleep on it :D If you really want Python 3.5 support I'll change pytest to something else. I just picked the first best thing that seemed simple to use. Also, with the new setuptools it is possible to make wheel packages with bdist_wheel, which seems to be the future. And I changed my mind about Tox. It's actually good :D

Your thoughts and comments would be greatly appreciated at this time. I made this rewrite just for fun, perhaps you don't like it at all? :)

meganomic commented 3 years ago

Okay, I think I'm done now. It was very pretty and neat until I had to implement the streaming mode. Tarfile doesn't provide any extensibility for that mode so I had to overload some functions.

xtarfile should now have feature parity with tarfile. It should be a complete drop in replacement. Unless I made any mistakes ofc :D

I still need to fix version.txt not working before merge.

meganomic commented 3 years ago

That took way too long to figure out lol.

I have altered cd.yml. Please verify using testpypi that it works correctly before using it.

meganomic commented 3 years ago

Would it be possible to refactor this to keep the previous code organization where we have all the code belonging to each new compression format in a separate file (zstandard.py, lz4.py) and then have the xtarfile.py file simply assemble them all together, e.g. via a mixin approach?

That is desirable. One issue though. The Stream mode does NOT support extension. The current way it's done is by overloading the functions in the _Stream and _StreamProxy class with copies of those functions with our added compression formats. This is simple, but annoying to extend.

A better way to do it would be to rewrite those function overloads to automagically add whatever compression is in (zstandard.py, lz4.py) or any other format that might be added in the future. Probably based on what's added to the OPEN_METH variable.

I wonder if it's possible to automagically add inheritance? Would be nice to remove all compression specific code from xtarfile.py. Being able to just drop in compression.py and have it automatically work would be nice

meganomic commented 3 years ago

Turns out that yes, you can automatically add inheritance. I think this is the way forward. I moved the zst.py and lz4.py to a sub-folder to make it easier to parse. Now you can just drop in a file there and it will automatically be loaded by xtarfile.

The Stream mode remains to be solved but it shouldn't be that difficult now that I know how to do the hard part.

meganomic commented 3 years ago

@c-w I think I'll give up on this PR. The streaming mode not being extendable kind of ruined the entire idea behind this PR. I didn't realize at the time that it would require such cancerous code. Well whatever. It was a fun learning experience if nothing else.

c-w commented 3 years ago

Yes, I agree. Thanks for this exploration though. Really good work and ideas in general. Sad it didn't work out.