6
\$\begingroup\$

I'm still pretty new at Python and working on a small database program, where at the start of the program new files are imported from another folder (approx. 2000), and older files are backed up. Subfolders of the import folder sometimes contain multiple files with the same name (I always want just one of them, but the newest version isn't always the one I want). This is an unfortunate situation that I don't have control over.

Because I don't have that much experience yet, I find it very difficult to judge if this is a "valid" way of doing it, and it would be great to have some feedback as for its readability and general usability.

The transfer-settings referred to in the script:

# settings.py

class FileTransferSettings:
    def __init__(self):
        self.input_folder: str = r"./input"
        self.search_subdirectories: bool = True

        self.output_folder: str = r"./output"
        self.backup_folder: str = r"./backup"
        self.report_folder: str = r"./reports"

        self.file_type_filter: list | None = [".yaml", ".csv"]
        self.file_name_filter: str | None = "standardized"


transfer_settings = FileTransferSettings()

The file transfer script:

# file_transfer.py

from collections import defaultdict
import os
from pathlib import Path
import shutil
import time
from logs import file_transfer_log
from settings import transfer_settings as ts


def _find_valid_files():
    def generate_glob_search_patterns() -> list[str]:
        file_filters = (ts.file_type_filter, ts.file_name_filter)

        match file_filters:
            case (None, None):
                return ["*"]
            case (None, ts.file_name_filter):
                return [f"*{ts.file_name_filter.lower()}*"]
            case (ts.file_type_filter, None):
                return [f"*{file_type}" for file_type in ts.file_type_filter]
            case (ts.file_type_filter, ts.file_name_filter):
                return [f"*{ts.file_name_filter.lower()}*{file_type}" for file_type in ts.file_type_filter]

    def find_all_matching_files(pattern_list: list[str]) -> dict[str, list[Path]]:
        result = defaultdict(list)

        for pattern in pattern_list:
            if ts.search_subdirectories:
                for path in Path(ts.input_folder).rglob(pattern):
                    if path.is_file():
                        result[path.name].append(path)
            elif not ts.search_subdirectories:
                for path in Path(ts.input_folder).glob(pattern):
                    if path.is_file():
                        result[path.name].append(path)
        return result

    def resolve_duplicate_file_names(path_object_dict: dict[str, list[Path]]) -> list[Path]:
        def print_duplicate_file_information(paths_to_print: list[Path]):
            print(f"\nDuplicate file names found:\n")
            for i, path in enumerate(paths_to_print):
                print(f"{i}:\n"
                      f"Full path      : {path.resolve()}\n"
                      f"Last modified  : {time.ctime(os.path.getmtime(path))}\n")

        def choose_file_to_transfer(paths_to_choose_from: list[Path]) -> int:
            while True:
                user_choice = input("Which file do you want to transfer? Input number here: ")

                if not user_choice.isnumeric():
                    print("Invalid input, input a number.\n")
                elif int(user_choice) < 0 or int(user_choice) > len(paths_to_choose_from) - 1:
                    print("Invalid input, number is not available.\n")
                else:
                    return int(user_choice)

        result = []
        for path_name in path_object_dict:
            path_objects_with_same_name = path_object_dict[path_name]
            path_index = 0
            if len(path_objects_with_same_name) > 1:
                print_duplicate_file_information(path_objects_with_same_name)
                path_index = choose_file_to_transfer(path_objects_with_same_name)
            result.append(path_objects_with_same_name[path_index])
        return result

    glob_patterns = generate_glob_search_patterns()
    path_list = resolve_duplicate_file_names(find_all_matching_files(glob_patterns))
    return path_list


def _transfer_file(path: Path):
    def is_file_to_transfer_newer(file_to_transfer: Path) -> bool:
        duplicate_file_in_output_folder = f"{ts.output_folder}/{file_to_transfer.name}"
        if os.path.getmtime(file_to_transfer) > os.path.getmtime(duplicate_file_in_output_folder):
            return True

    def backup_old_file(path_to_transfer: Path):
        file_to_backup = Path(f"{ts.output_folder}/{path_to_transfer.name}")
        filename, extension = file_to_backup.stem, file_to_backup.suffix
        version_number = 1

        while True:
            if os.path.exists(f"{ts.backup_folder}/{filename}_version_{version_number}{extension}"):
                version_number += 1
            else:
                backup = file_to_backup.rename(f"{ts.output_folder}/{filename}_version_{version_number}{extension}")
                shutil.move(backup, ts.backup_folder)
                break

    if not os.path.exists(f"{ts.output_folder}/{path.name}"):
        shutil.copy2(path, ts.output_folder)
        file_transfer_log.add_new(path)
    else:
        if is_file_to_transfer_newer(path):
            backup_old_file(path)
            shutil.copy2(path, ts.output_folder)
            file_transfer_log.add_updated(path)
        else:
            file_transfer_log.add_old(path)


def transfer_files():
    for file_path in _find_valid_files():
        _transfer_file(file_path)
\$\endgroup\$

1 Answer 1

6
\$\begingroup\$

use Path

        self.input_folder: str = r"./input"

The raw r-string is odd, unless you translated this from a Windows r".\input" directory.

You're clearly going for type safety, which is a laudable goal. It allows mypy and your IDE to help you out, offering hints. Consider adding a pathlib import, and then assign Path("./input"), or more simply just Path("input").

Path is a good way to tell the Gentle Reader that we're talking about something in the filesystem, plus it offers some nice convenience functions.


use top-level functions

You are clearly going to some trouble to organize the code, with _locally scoped identifiers and nested function defs. Which is laudable. But I usually try to steer clear of nested functions for at least two reasons.

  1. The parent function's local variables are in-scope for a nested function, which can be convenient but may lead to the usual coupling problems that global variables suffer from.
  2. A nested function is needlessly difficult to unit test.

I don't go so far as to say one should never nest. Just think carefully and write down the reason(s) you're using nesting instead of other techniques. Here are two techniques you might use in this codebase.

  1. Turn each top-level function into a module that contains the various helper functions.
  2. If you notice "same variables" being repeatedly passed around, turn a top-level function into a class and access those variables as self attributes.

namedtuple

Kudos on making file_filters a tuple (rather than a list). You might find it convenient to define a namedtuple or dataclass.

from collections import namedtuple

FileFilter = namedtuple("FileFilter", ("name", "type"))

The code sometimes mentions the file type first, which feels backwards.

There seems to be a "filenames shall be lowercase" invariant. Let's enforce that at assignment time, so we don't need to remember a .lower() call at use time. Similar remarks for when print_duplicate_file_information has to remember to .resolve() -- you might prefer to do that at assignment time.

The match technique is interesting. But maybe it's overkill. We could deal with name, then deal with type, then return the combined result. Then there would be a little less code duplication.

Using a None sentinel to denote "no filtering" is a perfectly sensible choice. But you may find denoting that with "*" to be more convenient. (And then adjust the Optional aspect of the type hint.)


"else" suffices

In find_all_matching_files this is needlessly hard to read:

            elif not ts.search_subdirectories:

Simplify it down to else: and be done with it.

We see three lines of code, twice, with the only difference being .rglob() / .glob(). Assign the desired glob variant to a variable, and make a function call indirect via that variable.


use convenience methods

f"Last modified  : {time.ctime(os.path.getmtime(path))}\n")

That makes perfect sense for a str path. nit: Given a Path path, the usual idiom would be path.stat().st_ctime. Similar remark for is_file_to_transfer_newer.

From a UX perspective it seems slightly odd to not print small integer indexes next to the file details.

Back in find_all_matching_files, notice that .glob() doesn't sort. The UX might be improved if glob'd files were always consistently displayed in sorted() order.


put types in type hints, not the name

You're definitely shooting for clarity when you invent new identifiers, and that's admirable. Some of them get to be a bit verbose, which doesn't help the reader. For example, rather than assigning path_objects_with_same_name, paths_with_same_name would suffice. Use a type hint if you want to make it explicit that we'll find a Path rather than a str in that container.

For that particular identifier, consider dup_paths.

Interestingly, I usually request that an author write more """docstrings""" in their code, yet I didn't feel that way about the current submission. Functions are small and names are clear. Nonetheless, writing a """single sentence""" on your public API entry points wouldn't hurt.


add automated tests

There is some slightly involved logic here. If you were to refactor some code, you would want the ability to quickly tell it is working properly, before and after a change.

Define a TestFileTransfer class that inherits from TestCase. Verify some aspect like glob'ing or obtaining ctime. Notice that there will never be a unit test for choose_file_to_transfer, and that is perfectly fine.


This code is well structured, showing attention to detail and to readability. It adheres to the single responsibility principle. Fine work. Keep it up!

I would not be willing to delegate or accept maintenance tasks on this codebase in its current form, as a maintainer could not add a small test in order to work on a small feature. If the recommended class or module refactoring is adopted to de-nest some functions, then I would be happy to delegate or accept such tasks.

\$\endgroup\$
4
  • \$\begingroup\$ Personally, I would argue that all those functions (including the sub-functions which might be made _private) should be on the FileTransferSettings (possibly renamed to FileTransferHelper or something) as well as passing actual arguments into __init__ probably not worth a separate answer though. \$\endgroup\$ Apr 3, 2023 at 18:31
  • \$\begingroup\$ @J_H This is really helpful advice, thank you! \$\endgroup\$ Apr 4, 2023 at 6:25
  • \$\begingroup\$ @DeathIncarnate What would be the advantage of doing so? \$\endgroup\$ Apr 4, 2023 at 6:27
  • 1
    \$\begingroup\$ @JanvanWijk Cleaner code structure in the sense of the related functions which are not intended to be used on anything but a FileTransferSettings are carried around with it. This means that I can create a FileTransferSettings with the appropriate initial arguments and it works there and then, and I can immediately find out what functions are related to it by checking its definition, (preferably) help or dir(obj). \$\endgroup\$ Apr 4, 2023 at 16:46

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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