OlavStornes / ModBuddy

Mod buddy is a mod manager created with extensibility in mind
GNU General Public License v3.0
35 stars 3 forks source link

Replace shutil with patool #12

Closed alan-cugler closed 1 year ago

alan-cugler commented 2 years ago

I am using ModBuddy for Linux game modding. Currently been playing StarSector alot, and the three main archives that are used by the modding community are: zip, rar, and 7z. In ModBuddy, shutil seems to cover zip files and tar balls. I recommend\request expanding the archive supported types to as many as possible as many different games will have modding communities using different compression tools.

https://github.com/OlavStornes/ModBuddy/blob/f049634c512c7d3e1666c5965f9ee5d620dce705/main.py#L267-L276

I recommend replacing the above with something like the below:

        for archive in archives[0]:
            suff = Path(archive).suffix
            folder_name = Path(archive).stem
            target_folder = Path(default_mod_folder) / folder_name
            try:
                patoolib.extract_archive(archive, outdir=target_folder)
            except FileNotFoundError:
                mod_home = Path(target_folder)
                mod_home.mkdir()
                patoolib.extract_archive(archive, outdir=target_folder)
            else:
                self.add_mod(target_folder)

I think my code is crap at the moment so I know the above proposed solution doesnt fully work or look pythonic. The main reason for switching to patool|patoolib is it does all the work for managing just about every archive type there is.

alan-cugler commented 2 years ago

I am adding the following two imports to main.py

import patoolib
from pathlib import Path

I am also adding patool to the requirments.txt file as a dependency.

alan-cugler commented 2 years ago

I am trying to make this improvement myself, but currently after successfully adding the new mod in with patools, ModBuddy doesnt seem to update its mods list in the json files and therefore refresh the GUI with the new mod row.

OlavStornes commented 2 years ago

Thanks for the issue. I'm glad to hear that this project is used in a game i'm patiently waiting for further development on.

Regarding patool, i'm noticing that the last release is back in 2016. As long as the package doesn't seem to create any issues with newer versions of python (>=3.9), I'd be thrilled to extend this functionality.

In the example you provided, does the folder actually extracts to the correct place? My suspicion is that patooolib throws FileNoutFoundError, which would prevent calling self.add_mod() in this case (else-clauses in a try-block only fires if there's no handled/unhandled exceptions).

alan-cugler commented 2 years ago

You are correct about FileNotFoundError, I have reworked the above pseudo code to a working improvement. I just haven't cleaned it up yet to do a PR. After talking with another developer I went with a code solution that remove the try clause and has the target_directory created just before the patoolib extraction.

The main reason i decided this was good enough is because FileNotFoundError shouldn't be the error to populate if the directory is made just before instantiation. There are a other reasons to have a try/except there for the archive extraction, but I wanted to focus on the improvement.

alan-cugler commented 2 years ago

If you feel like assigning this to me, I expect to have a PR here at some point in the week.

OlavStornes commented 2 years ago

Glad to hear that. Feel free to provide a solution when ever you have the time, and I'll be happy to look over it.

Regarding removal of the try clause: I'm hesitant to remove that in this case, however it isn't there because of a missing file (judging by your draft which is using FileNotFoundError), but because of potential errors with the archive, such as something unexpected errors regarding the archive itself. For instance, Shutil.ReadError can occur when an archive is unable to extract for an unspecified reason, such as corrupt data (even though my implementation heavily hints that it is due to an unsupported archive).

alan-cugler commented 2 years ago

That was my inference. With patoolib taking over for shutil I knew that particular pain should not populate. Though the corrupted archive is a real risk.