Python Enum Strategy Pattern

Question

I have two behaviours: pulling or cloning a git repository. It started as an Enum, but then I had to switch over the enum all the time. I wanted to turn it into some kind of strategy pattern and fiddled around with the declaration of the enum values. I found the following solution, but I have two questions regarding it:

1. Is it pythonic? If not, how cursed is it?
2. Why do I have to call action(params) instead of action.value(params)?

If I do the second, it says that .value is not defined on a function. However, it is the usual way to access an Enums value, and isn't the function the assigned value?

class GitAction(Enum):
    @staticmethod
    def clone(project_url: str, to_path: Path):
        logger.debug(f'Cloning {project_url} to {to_path}')
        Repo.clone_from(project_url, to_path)

    @staticmethod
    def pull(_, local_repo_path: Path):
        logger.debug(f'Pulling repo at {local_repo_path}')
        Repo(local_repo_path).remote().pull()

    CLONE = clone
    PULL = pull


def path_exists(path: Path) -> bool:
    return path.exists()


def get_required_git_action(repo_id: str, user_repo_root_directory: Path) -> GitAction:
    user_repo_path = user_repo_root_directory / repo_id
    if not path_exists(user_repo_path):
        return GitAction.CLONE
    else:
        return GitAction.PULL


def download_repo(username: str, repo_name: str, user_repo_root_directory=user_repository_download_location):
    repo_id = f'{username}-{repo_name}'
    user_repo_location = user_repo_root_directory / repo_id
    action = get_required_git_action(repo_id, user_repo_root_directory)
    logger.debug(f'downloading {repo_id} to {user_repo_location}')
    action(create_repo_url(username, repo_name), user_repo_location)
    logger.debug(f'downloaded {repo_id}')

I've been staring at the code base for some time and may take some things for granted.

Also, feel free to give feedback regarding readability/understandability of the code, I've been trying to give meaningful and intuitive names.

Answer 1

is it pythonic?

Ehhhhh. If we're feeling purist Zen, then probably not.

how cursed is it?

Functionally it's not reprehensible. About the most concerning factor is that it's very strange, and not something I ever see done. Another wart is that you're forcing a square peg into a round hole with your function signatures; clone needs a URL and a path but pull only needs a path.

There are many ways to refactor this. If I had to pick the one that would surprise me the least, then perhaps:

from pathlib import Path


class RepoManager:
    def __init__(
        self,
        username: str,
        repo_name: str,
        repo_root_dir: Path=USER_REPO_DOWNLOAD_LOCATION,
    ) -> None:
        self.username = username
        self.repo_name = repo_name
        self.repo_root_dir = repo_root_dir

    @property
    def repo_id(self) -> str:
        return f'{self.username}-{self.repo_name}'

    @property
    def repo_location(self) -> Path:
        return self.repo_root_dir / self.repo_id

    @property
    def repo_url(self) -> str:
        raise NotImplementedError('it is a mystery')

    def clone(self) -> None:
        # use self.repo_url and self.repo_location
        raise NotImplementedError('it is a mystery')

    def pull(self) -> None:
        # Only use self.repo_location
        raise NotImplementedError('it is a mystery')

    def get(self) -> None:
        if self.repo_location.exists():
            self.pull()
        else:
            self.clone()

Answer 2

General

1. I would not certainly call this strategy pattern. Strategy pattern lets you swtich the algorithm, not the semantics of the code. For example, your clone method could have two strategies viz clone via ssh or clone via https. Pulling from a git repo and cloning a git repo are two distinct operations that take different inputs and produce different outputs. This is quite clear from the interface as well. The pull method completely ignores the project_url.
2. This is just a code snippet. So, I cannot comment on the overall application. But if you have a requirement of representing two completely different behaviors as a same interface. There is something wrong in the overall architecture.
3. I dont know of anyone who stores a git repo in a folder named {username}-{repo_name}. People generally store it in a folder named {repo_name}.
4. There is absolutely no need to have seperate class members CLONE and PULL. The clone and pull methods are good enough.

Style

1. As far as the style goes, it is quite pythonic and should pass any PEP8 guidelines check.
2. Wrapping standard library calls in your own custom function is generally as bad practice and goes against There should be one way ... to do it. Consider replacing path_exists(user_repo_path) with user_repo_path.exists()

Answer 3

Whenever I see a function whose body contains a single line of simple code, I wonder why that function was written. For example,

def path_exists(path: Path) -> bool:
    return path.exists()

What is the purpose of this? Now instead of user_repo_path.exist() you need to write path_exists(user_repo_path), which is longer! And anyone looking at your code must figure out what this path_exists function does (the name is pretty clear, but I'd be reading it anyway, you never know if there are side-effects).

Considering how short and simple the logic is, I'd write just a single function:

def download_repo(
        username: str,
        repo_name: str,
        user_repo_root_directory: Path = user_repository_download_location,
):
    repo_id = f'{username}-{repo_name}'
    user_repo_location = user_repo_root_directory / repo_id
    if not user_repo_location.exist():
        repo_url = create_repo_url(username, repo_name)
        logger.debug(f'Cloning {repo_url} to {user_repo_location}')
        Repo.clone_from(repo_url, user_repo_location)
    else:
        logger.debug(f'Pulling repo at {user_repo_location}')
        Repo(user_repo_location).remote().pull()
    logger.debug(f'downloaded {repo_id}')

I personally find it easier to maintain code in a single function than code split across multiple functions and classes, when the logic is so simple and short.

For example, when I combined your code into one function, I realized that there’s a redundant logging call: you report the same information twice before downloading. That is the sort of issue you get more often when code is split up too much. You lose the overview.

The two lines that determine user_repo_location should definitely be a separate function if that same path is used elsewhere in the code. You wouldn't want that logic repeated. But if it's only used in this function, there is no need to separate it out because it's uncomplicated.

---

Note you are missing a type annotation for the 3rd input argument to download_repo. When you have more than two input arguments, or if they have long names or complex types, it is worth while to split the function's argument list over multiple lines, with one argument per line. This makes it much easier to read. I don't notice this annotation was missing until I reformatted the function declaration for the code block in this answer.

---

I don't know what create_repo_url() is, but is it able to figure out which of the many repo sources to find this project in? Or is this only used for repos in a specific private repository?