search

pytti-tools / pytti-core

https://pytti-tools.github.io/pytti-book/intro.html
MIT License
78 stars 23 forks source link

at least patch backups if not fix it #205

Closed dmarx closed 2 years ago

dmarx commented 2 years ago

https://discord.com/channels/869630568818696202/869675528494391307/988254689910984744

Bardia323 commented 2 years ago

The issue was compatability of the subprocess with Windows. Basically a syntax error. To resolve it, first: import platform which we use to find out the platform the code is being run in.

Then replace lines 145 to 150 with: if platform.platform().startswith("Win"): subprocess.run(["del", f"backup\\{file_namespace}\\{base_name}_{n-backups}.bak"] , shell=True, cwd=os.getcwd()) else: subprocess.run(["rm", f"/backup/{file_namespace}/{base_name}_{n-backups}.bak"] , shell=True)

dmarx commented 2 years ago

better yet, let's just use os.remove (which is cross-platform because it's native python) and drop the subprocess invocation entirely

Bardia323 commented 2 years ago

Yep! Good idea! 👍

On Mon., Jun. 20, 2022, 3:03 a.m. David Marx, @.***> wrote:

better yet, let's just use os.remove https://docs.python.org/3/library/os.html#os.remove (which is cross-platform because it's native python) and drop the subprocess invocation entirely

— Reply to this email directly, view it on GitHub https://github.com/pytti-tools/pytti-core/issues/205#issuecomment-1160052597, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACSJ7GJA5PRRN7KVWWBMXYLVQAJSTANCNFSM5ZHI5SQA . You are receiving this because you were assigned.Message ID: @.***>

dmarx commented 2 years ago

Should I look out for a PR? It's pretty easy if you've never submitted one before.

dmarx commented 2 years ago

allrighty, merged into main! could you do me a favor and confirm that the fix worked? I don't have a windows system to test on.

Bardia323 commented 2 years ago

I checked last night and didn't have any issues. But I'll double check and let you know tonight

On Mon., Jun. 20, 2022, 1:52 p.m. David Marx, @.***> wrote:

allrighty, merged into main! could you do me a favor and confirm that the fix worked? I don't have a windows system to test on.

— Reply to this email directly, view it on GitHub https://github.com/pytti-tools/pytti-core/issues/205#issuecomment-1160708524, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACSJ7GO5UDJP36SSBMH5BL3VQCVVFANCNFSM5ZHI5SQA . You are receiving this because you were assigned.Message ID: @.***>