7
\$\begingroup\$

We have a large set of projects at work. It's very important we keep track of those and every so often we need to run a full back-up that's unrelated to the 'normal' back-ups IT is running (snapshots every day, week, month, etc.). Recently we had a big migration on all of the older projects. To make sure nothing is irreparably damaged during the migration (basically a version upgrade), everything is backed-up twice before the upgrade.

There are network drives and slow computers involved (so everything is IO-bound), which makes CPU usage largely irrelevant.

Current speed:

  • under a minute/GB (local-to-local)
  • around 3 minutes/GB (local-to-network)
  • around 4 minutes/GB (network -> local)
  • under 2 minutes/GB (network -> network)

We're talking 60+ GB for the source directory, spread over 7k folders and 180k files. If we could somehow read/write differently to make it easier on the disks and the network, that would be great.

I'm working on an application that eventually does 3 things:

  • backup
  • set a secondary program loose on the original data for the migration
  • clean-up

This question is purely concerned about the first part of that process, the rest of the story is purely to explain the context and motivation. It can be used stand-alone just fine, so it's ready for review.

This program will perform some basic sanity checks on the directories involved, verify the required space is available and perform a backup to as many locations as you like. At the end it prints how many bytes were written, for a quick visual check whether anything went wrong. It's important that meta-data like the dates of creation and last modification are kept intact.

I've split the code into somewhat sensible functions. Some are doing the heavy lifting, others are just helpers to avoid repeating myself. I'm not happy about how I've implemented the directory validation, the reporting and exception handling is shoddy at best and I'm sure other functions can be improved in terms of re-usability.

I've tried doing some fancy things on the directory checking so I could recursively determine the validity without any care whether I passed a string, a WindowsPath or a dictionary. Which failed horribly. I feel like I'm not optimally using pathlib, but considering I'm legacy tools (I was running into fairly odd PermissionError trouble with shutil while xcopy "just works") I'm not entirely sure that can be helped. I think all the readouts and at least part of the validation could be done with decorators, like here but that's still fairly magical to me.

I'm fairly sure it shouldn't all be in one file either, but considering I'd just take everything except main and put it in a utils.py, I hadn't gone that route yet. I'm open to ideas. Honestly I'm surprised my code still works, it doesn't look like it should. I've attempted (not shown) to prettify it with wrappers and decorators, but it seems like it would require a rewrite before that's going to work. As long as the data gets copied in a cleanly matter, the rest of the code can still be changed any which way. The spec isn't set in stone.

# -*- coding: utf-8 -*-
"""
Created on Wed May  5 13:10:37 2021

@author: Mast

Notes:
    # xcopy & psutil don't seem to handle pathlib.WindowsPath too well,
    # converting to string conveniently turns / into \\
    # shutil kept running into PermissionError where xcopy has no trouble
"""

import os
import sys
import subprocess
import psutil
import time

from datetime import datetime
from pathlib import Path


PROJECTS_SOURCE = Path('Z:/EPLAN_DATA/Gegevens/Projecten/')

BACKUP_DIRECTORIES = {
    'local': Path('C:/backups/eplanprojects'),
    'network': Path('N:/BackupsEplan')
}

XCOPY_ARGS = "/e /h /i /q /s /z"

MAX_BACKUPS = 3
SLEEP_TIME = 60

FORCE = True


def to_megabytes(size):
    """
    Parameters
    ----------
    size : int

    Returns
    -------
    str
        Turn Bytes into rounded MegaBytes.

    """
    return "{0} MB".format(round(size / 1000000.0, 2))


def get_directory_size(directory):
    """
    Parameters
    ----------
    directory : pathlib.WindowsPath

    Returns
    -------
    int
        Size of the contents of the directory in Bytes.

    """
    print("Calculating size of {0}".format(directory))
    size = 0
    for path, _, files in os.walk(directory):
        for file in files:
            file_path = os.path.join(path, file)
            size += os.path.getsize(file_path)
    print(to_megabytes(size))
    return size


def free_space(directory):
    """
    Parameters
    ----------
    directory : pathlib.WindowsPath

    Returns
    -------
    int
        Free space available on partition the directory belongs to.

    """
    return psutil.disk_usage(str(directory)).free


def terminate_program(additional_info=""):
    """
    Parameters
    ----------
    additional_info : str (OPTIONAL)

    Print additional_info, wait & exit.

    """
    print("Program terminated before completion.")
    if additional_info:
        print(additional_info)
    time.sleep(SLEEP_TIME)
    sys.exit()


def validate_directory(directory):
    """
    Parameters
    ----------
    directory : pathlib.WindowsPath

    Raise FileNotFoundError if directory does not exist.

    """
    if not directory.exists():
        print("Invalid directory: {0}".format(directory))
        raise FileNotFoundError


def verify_available_backup_space(source_size, backup_directories):
    """
    Parameters
    ----------
    source_size : int
    backup_directories : dict of pathlib.WindowsPath

    Validate backup directories exist and are big enough to hold source.
    Terminate on failure.

    """
    for backup_directory in backup_directories.values():
        validate_directory(backup_directory)
        backup_space_available = free_space(backup_directory)
        if source_size > backup_space_available:
            # WARNING: If multiple back-up locations are on the SAME partition,
            # this check is insufficient
            print("That's not going to fit.\nTarget: {0} available.\nSource: {1}".format(
                backup_space_available, source_size))
            terminate_program()


def backup_projects(source, backup_directories):
    """
    Parameters
    ----------
    source : pathlib.WindowsPath
    backup_directories : dict of pathlib.WindowsPath

    Returns
    -------
    list of int : bytes_written

    Perform backup of source directory to (multiple) backup directories.

    """
    bytes_written = []
    for backup_directory in backup_directories:
        if len([f.path for f in os.scandir(backup_directories.get(backup_directory)) if f.is_dir()]) >= MAX_BACKUPS:
            print("Amount of immediate subdirectories in ({0}) is higher or equal to maximum amount of backups ({1}) configured.".format(
                backup_directory, MAX_BACKUPS))
            if not FORCE:
                terminate_program()
            else:
                print("Backup forced. Continuing.")
        print(backup_directories[backup_directory])

        start_time = datetime.now()
        print("Start copy {0}".format(start_time))
        try:
            subfolder = "_{}".format(start_time).replace(
                ':', '-').replace(' ', '_').split('.')[0]
            print(subfolder)
            syscall = "xcopy {source} {destination}\\{subfolder} {args}".format(
                source=str(source),
                destination=str(backup_directories[backup_directory]),
                subfolder=subfolder,
                args=XCOPY_ARGS)

            print(syscall)
            subprocess.run(syscall, check=True)
        except PermissionError:
            print("Permission denied: {0}".format(syscall))
            terminate_program()
        end_time = datetime.now()
        print("Started: {0}\nFinished: {1}\nExecution time {2}".format(
            start_time,
            end_time,
            end_time - start_time)
        )
        bytes_written.append(get_directory_size(
            str(backup_directories[backup_directory]) + '\\' + subfolder))
    for value in bytes_written:
        print(to_megabytes(value))


def main():
    validate_directory(PROJECTS_SOURCE)
    projects_source_size = get_directory_size(PROJECTS_SOURCE)

    verify_available_backup_space(projects_source_size, BACKUP_DIRECTORIES)

    backup_projects(PROJECTS_SOURCE, BACKUP_DIRECTORIES)


if __name__ == "__main__":
    main()
    print("Press any key...")
    input()

Real output:

Calculating size of Z:\EPLAN_DATA\Gegevens\Projecten
60585.67 MB
C:\backups\eplanprojects
Start copy 2021-05-07 17:21:57.007150
_2021-05-07_17-21-57
xcopy Z:\EPLAN_DATA\Gegevens\Projecten C:\backups\eplanprojects\_2021-05-07_17-21-57 /e /h /i /q /s /z
178642 File(s) copied
Started: 2021-05-07 17:21:57.007150
Finished: 2021-05-07 21:41:52.366467
Execution time 4:19:55.359317
Calculating size of C:\backups\eplanprojects\_2021-05-07_17-21-57
60585.67 MB
N:\BackupsEplan
Start copy 2021-05-07 21:43:31.600948
_2021-05-07_21-43-31
xcopy Z:\EPLAN_DATA\Gegevens\Projecten N:\BackupsEplan\_2021-05-07_21-43-31 /e /h /i /q /s /z
178642 File(s) copied
Started: 2021-05-07 21:43:31.600948
Finished: 2021-05-07 23:31:07.970629
Execution time 1:47:36.369681
Calculating size of N:\BackupsEplan\_2021-05-07_21-43-31
60585.67 MB
60585.67 MB
60585.67 MB
Press any key...

Anything and everything is up for review. Nitpick away.

Python 3.8.5 on Windows 10 x64, no limitations on libraries this time and no need to be cross-platform.

\$\endgroup\$
4
\$\begingroup\$

Function Level Review

  • `# -*- coding: utf-8 -*-`
    

    In Python 3 the default encoding is UTF-8.

  • "{0} MB".format(round(size / 1000000.0, 2))
    

    Division in Python 3 is float division, which functions like casting the numerator and/or denominator to floats before dividing. As such you don't need to make the divisor a float.
    Note: If you ever need Python 2 style division you can use //.

  • 1000000

    I'm struggling to read the number. We have two options on how to make the number easier to read.

    1. We can use, in my opinion, ugly scientific notation.
      1e6

    2. We can use _ as a thousands separator.
      1_000_000

  • "{0} MB".format(round(size / 1000000.0, 2))
    

    We can round to two decimal places using format rather than calling round.

    "{:.2f} MB".format(size / 1_000_000)
    
    f"{size / 1_000_000:.2f} MB"
    

    Note: round and .2f act differently. round can show 1-2 decimal places, where .2f always shows 2 dp.

    >>> f"{round(1.0, 2)}"
    '1.0'
    >>> f"{1.0:.2f}"
    '1.00'
    
  • Parameters
    ----------
    directory : pathlib.WindowsPath
    

    If we ran a type checker over your documentation on my OS your code would result in type errors. Only specify the OS specific implementation if you need the OS specific implementation.

    An example of needing an OS specific path is if we are parsing a URL and we convert the URL path to a PurePosixPath. As using a PureWindowsPath would make the output invalid. For the code to be correct the type has to be the POSIX one.

  • Printing inside of get_directory_size makes the code less reusable. What if you want to get the size of a directory later, but you don't want to print the information?

    You can add an 'echo' flag to the function later to duct-tape a fix. However isn't a good solution. I find thinking of code as either 'library code' or 'application code' to make the most sense. Library code is code which is devoid of business requirements, like interacting with a user. Where application code is the duct-tape holding together a bunch of library code to make the application your user interacts with. However by not having a clear separation between library code and application code you're making everything application code, which is harder to reuse later.

    I personally put library code into separate files or packages (which could make good pip packages). However with the amount of code you have, you'd just have added complexity for no benefit.

  • Your function get_directory_size looks good. If we ignore the SRP violation mentioned previously.

    I'd personally use a comprehension and sum. As the code would be far more dense, and compact in a way I find easier to read.

    However, neither is better.

  • Your naming is pretty good. So much so, I think most of your docstrings aren't needed.

    I think the docstrings are quite monotonous. Which is absolutely fine for to_megabytes and get_directory_size. However has spilled over into terminate_program which I think could really do with a bit more explanation.
    Why is the code sleeping? Why does the code take extra information to print? What is the goal of the code? Why are you not using standard Python exceptions?

    As such I'd recommend identifying and paying special attention to the functions which shouldn't be monotonous. If you write documentation after the code I'd recommend writing the special documentation first. As I tend to find my documentation's quality tends to drop off toward the end. And, in all situations, I'd recommend having a think of what are the special functions after the first time you've written documentation.

  • def terminate_program(additional_info=""):
    if additional_info:
    

    I'd recommend using None, rather than a truthy test, to check if any additional information has been passed. For example terminate_program(False) and terminate_program("False") output different information. Additionally None is the standard 'do nothing' singleton.

  • print("Program terminated before completion.")
    sys.exit()
    

    sys.exit defaults the status code to 0 - meaning the program ran successfully. However the message indicates the program exited abruptly and wasn't a successful run. Which should be a non-zero status code. Whilst I can appreciate you probably have no use for the status code, if you ever change workflow having a correctly set status code will be a help.

    For example many Linux shell prompts display whether the previous command ran successfully from the status code.

    Example of my shell showing a command which failed.

    Status codes can be used by shells for control flow. For example lets say I want to know if a program ran successfully. In Fish I'd use:

    if python -m "print('doing something'); raise SystemExit(1)"
        echo "successful"
    else
        echo "unsuccessful"
    end
    
    doing something
    unsuccessful
    

    Again you're probably not invoking from the CLI otherwise you wouldn't be using the input on exit 'pattern'. So kind of useless right now.

  • print("Invalid directory: {0}".format(directory))
    raise FileNotFoundError
    
    def main():
        validate_directory(PROJECTS_SOURCE)
        projects_source_size = get_directory_size(PROJECTS_SOURCE)
    
        verify_available_backup_space(projects_source_size, BACKUP_DIRECTORIES)
    

    We can see you've opted to not use your custom terminate_program. And you're not putting the error message in the exception. As such I think we've highlighted an issue with the error handling.

    Why is terminate_program insufficient here? What can we do to allow you to rely on one way to handle errors?

  • print("Amount of immediate subdirectories in ({0}) is higher or equal to maximum amount of backups ({1}) configured.".format(
    backup_directory, MAX_BACKUPS))
    

    I think your format string is too long. As I can't see {1} and MAX_BACKUPS at the same time. Using an f-string here would resolve the length issue.

    As such, one of the things I dislike about linters is the stringent adherence to a max character length. Splitting the f-string up into separate chunks would hurt readability. But would pass some dumb len check. I think keeping the print and string on one line is good.

  • if len([f.path for f in os.scandir(backup_directories.get(backup_directory)) if f.is_dir()]) >= MAX_BACKUPS:
    

    I personally have two styles for a >= b.

    1. When an expression is on a single line always have a preference for the smaller expression on the left hand side of the line.

      I tend to notice long lines of code tend to have one thing which is long, and everything else tends to be short. For example your long thing is "os.scandir(backup_directories.get(backup_directory))", which is really easy to read and is quite simple. If we ignore the fact you're using a comprehension for a moment. Having the short, but important parts, on the left-hand side of the editor means we don't accidentally skip over important information in the right hand side. Like noticing the check is len(...) <= MAX_BACKUPS not len([... if f.is_dir() <= MAX_BACKUPS]). Skimming lines can cause some silly misreadings.

    2. Always have a preference for < over >.
      Having a preference for one over the other means you can focus less on the minute detail and more on reading. A friend challenged me to complete a test where I was asked a bunch of </> questions. The challenge wasn't reading the text, or knowing what < or > are. The challenge was not being tripped up by the questionnaire constantly flipping between the two.

      I've picked <, over >, as ordering the smallest to largest, left to right and top to bottom are most common. If you have a sorted list of numbers which would you commonly say:

      • 0, 5, 231, 1342

      • 1342, 231, 5, 0

    A simple rearrange would get us:

    if MAX_BACKUPS <= len([f.path for f in os.scandir(backup_directories.get(backup_directory)) if f.is_dir()]):
    

    However if f.is_dir() is still on the right hand side of your space taker, os.scandir(...). So I'd split the comprehension over multiple lines.

    if (MAX_BACKUPS
        <= len([
            f.path
            for f in os.scandir(backup_directories.get(backup_directory))
            if f.is_dir()
        ])
    ):
    

    And you should be able to see here is a pretty good example where you have one big space filler, and everything else is small.

  • for backup_directory in backup_directories:
    
    backup_directories.get(backup_directory)
    
    print(backup_directories[backup_directory])
    
    destination=str(backup_directories[backup_directory]),
    
    str(backup_directories[backup_directory]) + '\\' + subfolder))
    

    You should either make a variable or iterate over the items.

    for backup_directory, destination in backup_directories.items():
    
  • start_time = datetime.now()
    "_{}".format(start_time).replace(':', '-').replace(' ', '_').split('.')[0]
    

    I think formatting the datetime would make the code much easier to understand.

    f"_{start_time:%Y-%m-%d_%H-%M-%S}"
    
  • str(backup_directories[backup_directory]) + "\\" + subfolder
    

    I think the best part of pathlib is being able to use / to join path segments. You should use / for cross platform compatibility. And, / is much cleaner than + "\\" + or + os.path.sep +.

    str(backup_directories[backup_directory] / subfolder)
    

Note: Docstrings removed for brevity.

import os
import sys
import subprocess
import psutil
import time

from datetime import datetime
from pathlib import Path


PROJECTS_SOURCE = Path('Z:/EPLAN_DATA/Gegevens/Projecten/')
BACKUP_DIRECTORIES = {
    'local': Path('C:/backups/eplanprojects'),
    'network': Path('N:/BackupsEplan')
}

XCOPY_ARGS = "/e /h /i /q /s /z"
MAX_BACKUPS = 3
SLEEP_TIME = 60
FORCE = True


def to_megabytes(size):
    return f"{size / 1_000_000:.2f}"


def _get_directory_size(directory):
    return sum(
        os.path.getsize(os.path.join(path, file))
        for path, _, files in os.walk(directory)
        for file in files
    )


def get_directory_size(directory):
    print("Calculating size of {0}".format(directory))
    size = _get_directory_size(directory)
    print(to_megabytes(size))
    return size


def free_space(directory):
    return psutil.disk_usage(str(directory)).free


def terminate_program(additional_info=""):
    print("Program terminated before completion.")
    if additional_info:
        print(additional_info)
    time.sleep(SLEEP_TIME)
    sys.exit()


def validate_directory(directory):
    if not directory.exists():
        print("Invalid directory: {0}".format(directory))
        raise FileNotFoundError


def verify_available_backup_space(source_size, backup_directories):
    for backup_directory in backup_directories.values():
        validate_directory(backup_directory)
        backup_space_available = free_space(backup_directory)
        if source_size > backup_space_available:
            # WARNING: If multiple back-up locations are on the SAME partition,
            # this check is insufficient
            print("That's not going to fit.\nTarget: {0} available.\nSource: {1}".format(
                backup_space_available, source_size))
            terminate_program()


def backup_projects(source, backup_directories):
    bytes_written = []
    for backup_directory in backup_directories:
        if (MAX_BACKUPS
            <= len([
                f.path
                for f in os.scandir(backup_directories.get(backup_directory))
                if f.is_dir()
            ])
        ):
            print("Amount of immediate subdirectories in ({0}) is higher or equal to maximum amount of backups ({1}) configured.".format(
                backup_directory, MAX_BACKUPS))
            if not FORCE:
                terminate_program()
            else:
                print("Backup forced. Continuing.")
        print(backup_directories[backup_directory])

        start_time = datetime.now()
        print("Start copy {0}".format(start_time))
        try:
            subfolder = "_{}".format(start_time).replace(':', '-').replace(' ', '_').split('.')[0]
            print(subfolder)
            syscall = "xcopy {source} {destination}\\{subfolder} {args}".format(
                source=str(source),
                destination=str(backup_directories[backup_directory]),
                subfolder=subfolder,
                args=XCOPY_ARGS
            )
            print(syscall)
            subprocess.run(syscall, check=True)
        except PermissionError:
            print("Permission denied: {0}".format(syscall))
            terminate_program()
        end_time = datetime.now()
        print("Started: {0}\nFinished: {1}\nExecution time {2}".format(
            start_time,
            end_time,
            end_time - start_time
        ))
        bytes_written.append(get_directory_size(
            str(backup_directories[backup_directory] / subfolder)
        ))
    for value in bytes_written:
        print(to_megabytes(value))


def main():
    validate_directory(PROJECTS_SOURCE)
    projects_source_size = get_directory_size(PROJECTS_SOURCE)
    verify_available_backup_space(projects_source_size, BACKUP_DIRECTORIES)
    backup_projects(PROJECTS_SOURCE, BACKUP_DIRECTORIES)


if __name__ == "__main__":
    main()
    print("Press any key...")
    input()

Design Level Review

  • As touched on earlier; lets redesign your terminate_program and main functions.
    Using exceptions for control flow is very, very common in Python. So using one/ones here seems more than reasonable. Rather than calling into terminate_program, we can raise a custom exception, Terminate, or any exception under, say, Exception. Because we'll be raising exceptions we'll need to change main to integrate terminate_program.

    We then have a choice of options for how to handle the exceptions. We can print full tracebacks with the traceback library. We can print the message of the exception with print. Or we can just not print anything.

    Because I commonly use setuptools entrypoints I have two mains. One to handle exceptions, and one to be the 'real' main.

    def _main():
        validate_directory(PROJECTS_SOURCE)
        projects_source_size = get_directory_size(PROJECTS_SOURCE)
        verify_available_backup_space(projects_source_size, BACKUP_DIRECTORIES)
        backup_projects(PROJECTS_SOURCE, BACKUP_DIRECTORIES)
    
    
    def main():
        try:
            _main()
        except Exception as e:
            print("Program terminated before completion.")
            message = str(e)
            if message:
                print(message)
            time.sleep(SLEEP_TIME)
            raise SystemExit(1) from None
        else:
            input("Press any key...\n")
    
    
    if __name__ == "__main__":
        main()
    
  • I think building a Backup class would improve the readability and reusability of your code.

    if (MAX_BACKUPS
        <= len([
            f.path
            for f in os.scandir(destination)
            if f.is_dir()
        ])
    ):
    

    Could become a single line with a call to a method count_child_dirs.

    f"_{datetime.datetime.now():%Y-%m-%d_%H-%M-%S}"
    
    syscall = f"xcopy {source} {destination / subfolder} {XCOPY_ARGS}"
    print(syscall)
    subprocess.run(syscall, check=True)
    

    Both can be hidden behind a method.

  • I think using logging rather than print would be a good idea. The line between when logging is good and when print is good isn't always clear. However here your prints seem more like a log of what happened, rather than, say, IO between the user and a game. By using logging too you could log to a file so you no longer need input or to be around when the script is running in case an error occurs.

    Note: I've not implemented logging and I've deleted some prints. Please ignore.

Here is a rewrite to show my train of thoughts.
Note: Documentation removed for brevity.

import collections
import datetime
import os
import pathlib
import subprocess
import time
from typing import Sequence, Union

import psutil

PROJECTS_SOURCE = pathlib.Path('Z:/EPLAN_DATA/Gegevens/Projecten/')
BACKUP_DIRECTORIES = {
    'local': pathlib.Path('C:/backups/eplanprojects'),
    'network': pathlib.Path('N:/BackupsEplan')
}

XCOPY_ARGS = "/e /h /i /q /s /z"
MAX_BACKUPS = 3
SLEEP_TIME = 60
FORCE = True

APath = Union[str, pathlib.PurePath]


def bytes_to_megabytes(size):
    return f"{size / 1_000_000:.2f}"


def get_directory_size(directory: APath) -> int:
    return sum(
        os.path.getsize(os.path.join(path, file))
        for path, _, files in os.walk(directory)
        for file in files
    )


def get_free_space(directory: APath) -> int:
    return psutil.disk_usage(str(directory)).free


class Backup:
    def __init__(self, path: pathlib.Path) -> None:
        self.path = path.resolve()
    
    def exists(self) -> bool:
        return self.path.exists()
    
    def count_child_dirs(self) -> int:
        return sum(
            1
            for f in os.scandir(self.path)
            if f.is_dir()
        )
    
    def disk(self) -> pathlib.Path:
        return pathlib.Path(self.path.drive)
    
    @staticmethod
    def backup_name(date: datetime.datetime) -> str:
        return f"_{date:%Y-%m-%d_%H-%M-%S}"
    
    def copy_from(self, source: APath, date: datetime.datetime) -> None:
        dir = self.path / self.backup_name(date)
        try:
            syscall = f"xcopy {source} {dir} {XCOPY_ARGS}"
            subprocess.run(syscall, check=True)
            return dir
        except PermissionError:
            raise ValueError(f"Permission denied: {syscall}")


def validate_backups(source_size: int, backups: Sequence[Backup]) -> None:
    disks = collections.defaultdict(int)
    for backup in backups:
        disks[backup.disk()] += 1
        if not backup.exists():
            raise FileNotFoundError(f"Invalid directory: {backup}")
        if not FORCE and MAX_BACKUPS <= backup.count_child_dirs():
            raise ValueError(f"Amount of immediate subdirectories in ({backup}) is higher or equal to maximum amount of backups ({MAX_BACKUPS}) configured.")
    for disk, amount in disks.items():
        if amount * get_free_space(disk) < source_size:
            raise ValueError(f"That's not going to fit.\nTarget: {disk} available.\nSource: {source_size}")


def backup_projects(source: APath, backups: Sequence[Backup]) -> None:
    bytes_written = []
    for backup in backups:
        print(backup)
        start_time = datetime.datetime.now()
        print(f"Start copy {start_time}")
        dir = backup.copy_from(source)
        end_time = datetime.datetime.now()
        print(f"Started: {start_time}\nFinished: {end_time}\nExecution time {end_time - start_time}")
        print(f"Calculating size of {dir}")
        size = get_directory_size(dir)
        print(bytes_to_megabytes(size))
        bytes_written.append(size)
    for value in bytes_written:
        print(bytes_to_megabytes(value))


def _main():
    if not PROJECTS_SOURCE.exists():
        raise FileNotFoundError(f"Invalid directory: {PROJECTS_SOURCE}")
    print(f"Calculating size of {PROJECTS_SOURCE}")
    projects_source_size = get_directory_size(PROJECTS_SOURCE)
    print(bytes_to_megabytes(projects_source_size))
    backups = [Backup(b) for b in BACKUP_DIRECTORIES.values()]
    validate_backups(projects_source_size, backups)
    backup_projects(PROJECTS_SOURCE, backups)


def main():
    try:
        _main()
    except Exception as e:
        print("Program terminated before completion.")
        message = str(e)
        if message:
            print(message)
        time.sleep(SLEEP_TIME)
        raise SystemExit(1) from None
    else:
        input("Press any key...\n")


if __name__ == "__main__":
    main()
\$\endgroup\$
4
  • \$\begingroup\$ Great answer! Just a quick comment, is there a reason you are using os.walk instead of doing something like sum(f.stat().st_size for f in path.rglob("*") if f.exists())? Cheers \$\endgroup\$ Jun 11 '21 at 23:19
  • 2
    \$\begingroup\$ @N3buchadnezzar in get_directory_size? The simple answer is because Mast was already used os.walk. The longer answer is because I prefer using os.walk when I have to recursively descend and I only want directories or files. Having to figure out which of f.is_dir(), f.is_file(), f.is_symlink(), etc I want isn't how I want to spend my time. I also don't really want to figure out if rglob walks symlinks, and if rglob does how can I stop walking symlinks? os.walk is good enough \$\endgroup\$
    – Peilonrayz
    Jun 11 '21 at 23:29
  • \$\begingroup\$ Makes sense =) If I am already importingpathlib I try to get the most out of it before importing os, I guess that is just a bad habit of mine. Using subprocess with pathlib usually gets the job done. Speedwise I think they are much the same. \$\endgroup\$ Jun 11 '21 at 23:34
  • 1
    \$\begingroup\$ @N3buchadnezzar I confess when I first used pathlib I did the same :) Everything and everything was pathlib, os.path no more. But I now try to use the best tool for the job rather than being a 'purist'. For example, os.path.commonprefix is pretty handy at times. Not necessarily when I'm interacting with paths mind you ;) \$\endgroup\$
    – Peilonrayz
    Jun 12 '21 at 1:12
2
\$\begingroup\$

You can use f-strings instead of .format(). It makes the code more readable, and is also faster (see here, and even on my system, f-strings are about 4.4 times faster than .format() on my Windows machine, and about 3.5 times faster on WSL2 with Ubuntu 20.04 LTS), though the speed difference is negligible in most cases.

For example, in get_directory_size, you can write print(f"Calculating size of {directory}").

Similarly, in to_megabytes, you can write return f"{round(size / 1_000_000.0, 2)} MB".

\$\endgroup\$
2
  • 1
    \$\begingroup\$ f-strings have the side-effect of handling backslashes a little differently like I do in "Started: {0}\nFinished: {1}\nExecution time {2}".format(, but apparently there are workarounds. \$\endgroup\$
    – Mast
    May 8 '21 at 15:38
  • \$\begingroup\$ @Mast Yes, it is true that you cannot use backslashes inside the curly brackets in f-strings (thanks for pointing that out, I actually didn't know about that), but the example you posted is perfectly legal, even with f-strings. But you are correct: There are most definitely use-cases where it is better to use .format and stay away from f-strings. \$\endgroup\$
    – wermos
    May 9 '21 at 9:41

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.