0

I'm using argparse.ArgumentParser extensively; however, it comes with a lot of boilerplate to set up, and this is especially noticeable when you've got more than a few common arguments that probably exist between functionally different, but effectively the "same" update scripts.

In detail, I am writing a common update_utils library, that I use in (currently two) two python scripts. The purpose of this library is kind of generic and perhaps all over the place; it provides a common, sane framework as a "base" to work off without the need to duplicate alot of code between modules. More specifically, three core features are provided by this module:

  1. An ABC Manager, meant to help with the workflow of parsing the releases of a Git repository, only GitHub right now, which is supposed to be subclassed and used
  2. An ABC Distribution, which is subclassed several times in the update_utils library, in order to provide a couple of methods to help with managing system-level packages, and notifying the user as such
  3. An ArgumentParser "default" instance, that should resolve a few issues with boilerplate I've had (mentioned above) when using that particular python module.
class Distribution(ABC):
    # FIXME add documentation

    @abstractmethod
    def check(self, packages: list[Package]) -> list[Package]:
        # FIXME add documentation
        raise NotImplementedError

    @abstractmethod
    def install(self, packages: list[Package]) -> tuple[bool, str, str]:
        # FIXME add documentation
        raise NotImplementedError

NOTE: excuse the lack of implementation, the second bullet point as a concept is something I've started working on very recently, and I want to resolve some design issues while it's at it's infancy, rather than later. I promise this is code from a real project, this section's just not finished yet. To be honest, I'm facing issues here as well, and it'll probably be part of a different question all by itself.

This question is posted in regards to the bullet point 3.

import argparse as ap

DEFAULT_FLAGS = {
    "--version": {
        "help": "Specify a version to install. Default is latest.",
        "required": False,
        "default": None
    },
    "--keep": {
        "help": "Specify if downloaded files will be kept after finishing.",
        "required": False,
        "default": False,
        "action": "store_true"
    },
    "--temporary": {
        "help": "Specify temporary directory files will be downloaded at. Default is /tmp/",
        "required": False,
        "default": None,
        "type": str
    },
    "--destination": {
        "help": "Specify installation directory.",
        "required": False,
        "default": None,
        "type": str
    },
    "--unsafe": {
        "help": "Specify if file verification will be skipped. Set by default if unsupported by the repository.",
        "required": False,
        "default": False,
        "action": "store_true"
    },
}


def get_default_argparser(description:
    p = ap.ArgumentParser(description=description)
    for argname, argopts in DEFAULT_FLAGS.items():
        p.add_argument(argname, **argopts)
    return p

More specifically, it turns out another module that depends on this library, and the "default" implementation of agrparser, while 4 out of 5 flags (seen above) fit the glove and all, it turns out that during the implementation of a specific feature in a dependent module, the --temporary flag needs to support more complex behaviour rather than the one provided from the get_default_argparser function. Specifically, it requires a mutually exclusive group, with a few different options, here's the relevant excerpt from the dependent module:

import update_utils as ut
import os

STEAM_INSTALL_DIR = os.path.expanduser(
    "~/.local/share/Steam/compatibilitytools.d/"
)
STEAM_FLATPAK_INSTALL_DIR = os.path.expanduser(
    "~/.var/app/com.valvesoftware.Valve/.local/share/Steam/compatibilitytools.d/"
)
LUTRIS_INSTALL_DIR = os.path.expanduser(
    "~/.local/share/lutris/runners/wine/"
)
LUTRIS_FLATPAK_INSTALL_DIR = os.path.expanduser(
    "~/.var/app/net.lutris.Lutris/data/lutris/runners/wine/"
)

def create_argparser():
    p = ut.get_default_argparser(
        "Download & extract latest version of the most popular game compatibility layers.",
        destination=False
    )
    destination_group = p.add_mutually_exclusive_group(required=True)
    destination_group.add_argument(
        "--destination",
        help="Install @ custom installation directory",
        required=False,
        default=None,
        type=str
    )
    destination_group.add_argument(
        "--steam",
        help=f"Install @ default steam directory\n\t{STEAM_INSTALL_DIR}",
        required=False,
        default=None,
        action="store_true"
    )
    destination_group.add_argument(
        "--steam-flatpak",
        help=f"Install @ default steam (flatpak) directory\n\t{STEAM_FLATPAK_INSTALL_DIR}",
        required=False,
        default=None,
        action="store_true"
    )
    destination_group.add_argument(
        "--lutris",
        help=f"Install @ default lutris directory\n\t{LUTRIS_INSTALL_DIR}",
        required=False,
        default=None,
        action="store_true"
    )
    destination_group.add_argument(
        "--lutris-flatpak",
        help=f"Install @ default lutris (flatpak) directory\n\t{LUTRIS_FLATPAK_INSTALL_DIR}",
        required=False,
        default=None,
        action="store_true"
    )
    return p

Of course, if I tried to get the argparse implementation that included --destination without removing it from the DEFAULT_FLAGS beforehand, it would result in erroneous behaviour, or outright hard to locate runtime errors. Coming to the crux of the issue, I rewrote get_default_argparser to this:

from copy import deepcopy
import argparse as ap

DEFAULT_FLAGS = {
    "--version": {
        "help": "Specify a version to install. Default is latest.",
        "required": False,
        "default": None
    },
    "--keep": {
        "help": "Specify if downloaded files will be kept after finishing.",
        "required": False,
        "default": False,
        "action": "store_true"
    },
    "--temporary": {
        "help": "Specify temporary directory files will be downloaded at. Default is /tmp/",
        "required": False,
        "default": None,
        "type": str
    },
    "--destination": {
        "help": "Specify installation directory.",
        "required": False,
        "default": None,
        "type": str
    },
    "--unsafe": {
        "help": "Specify if file verification will be skipped. Set by default if unsupported by the repository.",
        "required": False,
        "default": False,
        "action": "store_true"
    },
}

def get_default_argparser(description, version=True, keep=True, temporary=True, destination=True, unsafe=True):
    flags_dict = deepcopy(DEFAULT_FLAGS)
    if not version:
        del flags_dict["--version"]
    if not keep:
        del flags_dict["--keep"]
    if not temporary:
        del flags_dict["--temporary"]
    if not destination:
        del flags_dict["--destination"]
    if not unsafe:
        del flags_dict["--unsafe"]

    p = ap.ArgumentParser(description=description)
    for argname, argopts in flags_dict.items():
        p.add_argument(argname, **argopts)
    return p

I don't think any amount of documentation is saving this function, and this is not only a code smell, but looks like a ticking time bomb in an expanding code base that needs to be flexible enough to support different requirements. However, I can't think of a good enough way to reimplement this without satisfying the following two requirements:

  • Somewhat concise: it requires less boilerplate than adding these arguments to every dependent (manually), and is less error prone
  • Self explanatory: it requires little to no explanation in order to understand how it works and what's going on behind the scenes

I thought about making a class that'll have to be passed as an argument to get_default_argparser, with inverse flags in it's constructor in order to eliminate unneeded behaviour (arguments), however I have a strong feeling this is 2 steps backwards, and it's probably not as self-explanatory as it sounds.

EDIT: J_H's answer is a great one and probably the ideal way for any other person reading this question; however this is not the ideal way to solve my specific issue in my specific usecase, and perhaps it's my fault I didn't disclose it in the first place. The reason I'm looking a design pattern and not to mirate to a modern & better library is twofold: these scripts intentionally run without importing external libraries (#!/usr/bin/env -S python3 -S -OO shebang) that aren't included with <=python3.12 for somewhat obscure reasons, but one of the benefits of "update-utils" I'm writing is the lack of third party libraries, making it super fast to spin up the python VM if need be; since this is a building block for the remainder of my scripts, I'd like for this option to remain if possible. The second reason J_H's answer doesn't fit my usecase, is because this module and subsequent modules meant to run in the global python ENV, and I would like to avoid contamination, or relying on external libraries. This is a little hypocritical from me, but the only external library all of these scripts depend on is requests, of which only update-utils interacts with; scripts should barely know of it's existence other than catching Exception(s). Like I said, if my usecase wasn't so super-specific, this is probably the "correct" answer; however I'm specifically looking for A Design Pattern to provide a modular, but scalable, "batteries-included" implementation of an instance of a class.

1
  • You created an abstraction (your utils library, the default-parser function) and are now struggling because it was the wrong abstraction. The wrong abstraction can be worse than no abstraction at all. Some duplication accross projects is inevitable. Because you don't actually have a set of default flags, I'd think about making it easier to add commonly used flags, like a function add_version_flag(p). That said, your get_default_argparser(**kwargs) function could also be made simpler by giving it a set of names to skip, e.g. without={"--destination"}
    – amon
    Commented Nov 16, 2023 at 7:50

2 Answers 2

2

I love argparse, I have used it in many projects.

Nowadays I usually use typer instead. It's just too easy. Mention some type annotations, and the --help comes "for free".

a good enough way to re-implement this ... [concise, self explanatory]

Yes, I agree with you. Defining a new TempDir class with the business logic you need is the appropriate way to model this.

Throw in a suitable type annotation

def main(output_folder: TempDir = TempDir('/tmp')) -> None:
    ...

if __name__ == '__main__':
    typer.run(main)

and you're halfway toward making your users happy.

2
  • 2
    +1 I was just going to comment on the question: Save yourself the trouble. Skip argparse and use typer.
    – JimmyJames
    Commented Nov 15, 2023 at 19:40
  • This is a good answer, however it doesn't fit my specific usecase; see edit, what I wanted to clarify was too much to write as a comment. As a sidenote, perhaps since it's late here, I don't understand the pattern you're describing; should I model the arguments of get_default_argparser with classes each one, or should I have a "central class" that describes the intended behaviour (as noted in my question)?
    – g_elef
    Commented Nov 15, 2023 at 20:05
1

I decided to go for a Builder pattern; like amon mentioned in the comments of the question, this was the wrong abstraction in the first place.

Here's the end result:

class ArgumentParserBuilder:
    DEFAULT_VERSION: tuple[(str, ...), dict[...]] = (
        ("-v", "--version"),
        {
            "help": "Specify a version to install.",
            "required": False,
            "default": None
        }
    )

    DEFAULT_KEEP: tuple[(str, ...), dict[...]] = (
        ("-k", "--keep"),
        {
            "help": "Specify if temporary file cleanup will be performed.",
            "required": False,
            "default": False,
            "action": "store_true"
        }
    )

    DEFAULT_TEMPORARY: tuple[(str, ...), dict[...]] = (
        ("-t", "--temporary"),
        {
            "help": "Specify temporary (download) directory files.",
            "required": False,
            "default": None,
            "type": str
        }
    )

    DEFAULT_DESTINATION: tuple[(str, ...), dict[...]] = (
        ("-d", "--destination"),
        {
            "help": "Specify installation directory.",
            "required": False,
            "default": None,
            "type": str
        }
    )

    DEFAULT_UNSAFE: tuple[(str, ...), dict[...]] = (
        ("-u", "--unsafe"),
        {
            "help": "Specify if file verification will be skipped.",
            "required": False,
            "default": False,
            "action": "store_true"
        }
    )

    def __init__(self, description: str):
        self.parser = ap.ArgumentParser(description=description)

    def add_version(self) -> Self:
        flags, kwargs = ArgumentParserBuilder.DEFAULT_VERSION
        self.parser.add_argument(*flags, **kwargs)
        return self

    def add_keep(self) -> Self:
        flags, kwargs = ArgumentParserBuilder.DEFAULT_KEEP
        self.parser.add_argument(*flags, **kwargs)
        return self

    def add_temporary(self) -> Self:
        flags, kwargs = ArgumentParserBuilder.DEFAULT_TEMPORARY
        self.parser.add_argument(*flags, **kwargs)
        return self

    def add_destination(self) -> Self:
        flags, kwargs = ArgumentParserBuilder.DEFAULT_DESTINATION
        self.parser.add_argument(*flags, **kwargs)
        return self

    def add_unsafe(self) -> Self:
        flags, kwargs = ArgumentParserBuilder.DEFAULT_UNSAFE
        self.parser.add_argument(*flags, **kwargs)
        return self

    def add_arguments(self, flags_kwargs_dict: dict[(str, ...), dict[...]]) -> Self:
        for flags, argopts in flags_kwargs_dict.items():
            self.parser.add_argument(*flags, **argopts)
        return self

    def add_mutually_exclusive_group(self, flags_kwargs_dict: dict[(str, ...), dict[...]], required=True) -> Self:
        meg = self.parser.add_mutually_exclusive_group(required=required)
        for flags, argopts in flags_kwargs_dict.items():
            meg.add_argument(*flags, **argopts)
        return self

    def build(self) -> ap.ArgumentParser:
        return self.parser

The class might look a little like a god class at first glance, however it's intent is very specific and results in pretty robust code as far as I can tell.

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.