4
\$\begingroup\$

My Code

import shutil
import os
from pathlib import Path
from datetime import datetime


DOWNLOAD_DIR = Path("/Users/my_name/Desktop/Zip Test/g_2020-12-10_3/")
ZIPPED_DIR = Path("/Users/my_name/Desktop/Zip Test/g_2020-12-10_3.zip")
ZIPPED_DIR.mkdir(exist_ok=True)


def make_archive(source, destination):
    delivery_file_name = destination.split("/")[-1]
    delivery_name = delivery_file_name.split(".")[0]
    format = delivery_file_name.split(".")[1]
    archive_from = os.path.dirname(source)
    archive_to = os.path.basename(source.strip(os.sep))
    shutil.make_archive(delivery_name, format, archive_from, archive_to)
    shutil.move(
        "%s.%s" % (delivery_name, format),
        "/Users/my_name/Desktop/Zip Test/zipped_content",
    )


make_archive(str(DOWNLOAD_DIR), str(ZIPPED_DIR))

Description

I am very new to python so i had a very hard time getting shutil.make_archive to do what i want. This code is finally working and i feel it is a little hard to read so i was wondering if someone can help me refactor this to simplify it. I don't want to loose the functionality of what i am doing. In my code i basically go to my /Users/my_name/Desktop/Zip Test and inside it is a folder called g_2020-12-10_3 which contains the contents i want to zip up so i do that.

When i unzip the contents it unzips folder named g_2020-12-10_3 and inside it are the contents.

I want to keep this functionality but at the same time simplify the code below and any help would be appreciated here.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ Why did you try to use pathlib, just to turn everything into a string and make it yourself? Pathlib provides all the APIs you need to do the kinds of operations you did \$\endgroup\$ Dec 10 '20 at 21:05
  • \$\begingroup\$ right, i am not as familiar with pathlib i know i can do somethings like rglob etc to look up files \$\endgroup\$
    – Dinero
    Dec 10 '20 at 21:12
2
\$\begingroup\$

You're using pathlib, which is great! Unfortunately, you immediately discard the functionality it gives you, and use old-style os APIs.

A good place to start is here - a mapping between the os[.path] APIs and their Path equivalents. Doing that gets us here:

def make_archive2(source, destination):
    shutil.make_archive(destination.stem, destination.suffix, source.parent, source.name)
    shutil.move(destination, "/Users/my_name/Desktop/Zip Test/zipped_content")

We now don't need to do any manual manipulations using os or os.path. Once we're here, we can actually realize that the first parameter to make_archive is a full path to the file, completely removing the requirement to call shutil.move

From there, we can improve some of the other things you do. For example:

  • Your source and destination are the same thing... but you don't actually use the destination in a meaningful way
  • You extract the format from the destination, which feels a bit odd
  • You hardcode the actual final destination
  • You use both the root_dir and base_dir parameters, when that really isn't necessary
  • You don't return where the file ended up, which feels nicer than the caller having to set up their Path themselves

Altogether, I ended up with something like this:

def make_archive3(to_archive, move_to, archive_format="zip"):
    move_to.mkdir(exist_ok=True)
    return shutil.make_archive(move_to / to_archive.name, archive_format, to_archive)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.