Problem
This question is a sequel to one I asked the other day. The previous question examined the core of my repo; this one examines the user-facing part. If you’re curious to see what the repo as a whole is trying to achieve, feel free to check out the previous question, but it shouldn’t be necessary for the purposes of this one.
As mentioned above, Python’s argparse is front and centre in the below code. This is literally the first time that I’ve used this package. The code seems to be working exactly as intended, but I’d like to check that I’m not ugly or painful in terms of style.
"""
This code serves as the entry point to this package.
"""
# Standard imports.
import argparse
# Local imports.
from git_credentials import set_up_git_credentials
from hm_software_installer import
HMSoftwareInstaller,
DEFAULT_OS,
DEFAULT_TARGET_DIR,
DEFAULT_PATH_TO_WALLPAPER_DIR,
DEFAULT_PATH_TO_GIT_CREDENTIALS,
DEFAULT_PATH_TO_PAT,
DEFAULT_GIT_USERNAME,
DEFAULT_EMAIL_ADDRESS,
DEFAULT_PYTHON_VERSION
# Constants.
ARGUMENTS = [
{
"name": "--os",
"default": DEFAULT_OS,
"dest": "this_os",
"help": "A string giving the OS in use on this system",
"type": str
}, {
"name": "--email-address",
"default": DEFAULT_EMAIL_ADDRESS,
"dest": "email_address",
"help": "Your email address",
"type": str
}, {
"name": "--git-username",
"default": DEFAULT_GIT_USERNAME,
"dest": "git_username",
"help": "Your Git username",
"type": str
}, {
"name": "--path-to-git-credentials",
"default": DEFAULT_PATH_TO_GIT_CREDENTIALS,
"dest": "path_to_git_credentials",
"help": "The path to the Git credentials file",
"type": str
}, {
"name": "--path-to-pat",
"default": DEFAULT_PATH_TO_PAT,
"dest": "path_to_pat",
"help": "The path to the Personal Access Token file",
"type": str
}, {
"name": "--path-to-wallpaper-dir",
"default": DEFAULT_PATH_TO_WALLPAPER_DIR,
"dest": "path_to_wallpaper_dir",
"help": (
"The path to the directory where the wallpaper images are held"
),
"type": str
}, {
"name": "--pip-version",
"default": DEFAULT_PYTHON_VERSION,
"dest": "pip_version",
"help": (
"The version of PIP you wish to use on this device"
),
"type": int
}, {
"name": "--python-version",
"default": DEFAULT_PYTHON_VERSION,
"dest": "python_version",
"help": (
"The (integer) version of Python you wish to use on this device"
),
"type": int
}, {
"name": "--target-dir",
"default": DEFAULT_TARGET_DIR,
"dest": "target_dir",
"help": (
"The path to the directory into which we're going to install stuff"
),
"type": str
}, {
"name": "--thunderbird-num",
"default": None,
"dest": "thunderbird_num",
"help": "The Thunderbird number for this device",
"type": int
}
]
BOOLEAN_ARGUMENTS = [
{
"name": "--reset-git-credentials-only",
"action": "store_true",
"default": False,
"dest": "reset_git_credentials_only",
"help": "Reset the Git credentials, but perform no installations"
}
]
####################
# HELPER FUNCTIONS #
####################
def add_boolean_arguments(parser):
""" Add the Boolean flags to the parser object. """
for argument in BOOLEAN_ARGUMENTS:
parser.add_argument(
argument["name"],
action=argument["action"],
default=argument["default"],
dest=argument["dest"],
help=argument["help"]
)
def make_parser():
""" Ronseal. """
result = argparse.ArgumentParser(description="Parser for HMSS")
for argument in ARGUMENTS:
result.add_argument(
argument["name"],
default=argument["default"],
dest=argument["dest"],
help=argument["help"],
type=argument["type"]
)
add_boolean_arguments(result)
return result
def make_installer_obj(arguments):
""" Ronseal. """
result =
HMSoftwareInstaller(
this_os=arguments.this_os,
target_dir=arguments.target_dir,
thunderbird_num=arguments.thunderbird_num,
path_to_git_credentials=arguments.path_to_git_credentials,
path_to_pat=arguments.path_to_pat,
git_username=arguments.git_username,
email_address=arguments.email_address,
path_to_wallpaper_dir=arguments.path_to_wallpaper_dir
)
return result
def run_git_credentials_function(arguments):
set_up_git_credentials(
username=arguments.git_username,
email_address=arguments.email_address,
path_to_git_credentials=arguments.path_to_git_credentials,
path_to_pat=arguments.path_to_pat
)
###################
# RUN AND WRAP UP #
###################
def run():
""" Run this file. """
parser = make_parser()
arguments = parser.parse_args()
if arguments.reset_git_credentials_only:
run_git_credentials_function(arguments)
else:
installer = make_installer_obj(arguments)
installer.run()
if __name__ == "__main__":
run()
Solution
I am not convinced, that using dicts to initialize the argument parser is sensible. Do you need the dicts anywhere else? If not, why not just pass the data in the dicts to the argument parser’s methods directly, i.e. why have them in the first place?
But if you insist on doing this, you can use dict unpacking to remove unneccessary code:
ARGUMENTS = [
{
"name": "--os",
"default": DEFAULT_OS,
"dest": "this_os",
"help": "A string giving the OS in use on this system",
"type": str
}, {
"name": "--email-address",
"default": DEFAULT_EMAIL_ADDRESS,
"dest": "email_address",
"help": "Your email address",
"type": str
}, {
"name": "--git-username",
"default": DEFAULT_GIT_USERNAME,
"dest": "git_username",
"help": "Your Git username",
"type": str
}, {
"name": "--path-to-git-credentials",
"default": DEFAULT_PATH_TO_GIT_CREDENTIALS,
"dest": "path_to_git_credentials",
"help": "The path to the Git credentials file",
"type": str
}, {
"name": "--path-to-pat",
"default": DEFAULT_PATH_TO_PAT,
"dest": "path_to_pat",
"help": "The path to the Personal Access Token file",
"type": str
}, {
"name": "--path-to-wallpaper-dir",
"default": DEFAULT_PATH_TO_WALLPAPER_DIR,
"dest": "path_to_wallpaper_dir",
"help": (
"The path to the directory where the wallpaper images are held"
),
"type": str
}, {
"name": "--pip-version",
"default": DEFAULT_PYTHON_VERSION,
"dest": "pip_version",
"help": (
"The version of PIP you wish to use on this device"
),
"type": int
}, {
"name": "--python-version",
"default": DEFAULT_PYTHON_VERSION,
"dest": "python_version",
"help": (
"The (integer) version of Python you wish to use on this device"
),
"type": int
}, {
"name": "--target-dir",
"default": DEFAULT_TARGET_DIR,
"dest": "target_dir",
"help": (
"The path to the directory into which we're going to install stuff"
),
"type": str
}, {
"name": "--thunderbird-num",
"default": None,
"dest": "thunderbird_num",
"help": "The Thunderbird number for this device",
"type": int
},
{
"name": "--reset-git-credentials-only",
"action": "store_true",
"default": False,
"dest": "reset_git_credentials_only",
"help": "Reset the Git credentials, but perform no installations"
}
]
####################
# HELPER FUNCTIONS #
####################
def make_parser():
""" Ronseal. """
result = argparse.ArgumentParser(description="Parser for HMSS")
for argument in ARGUMENTS:
result.add_argument(
argument.pop("name"),
**argument
)
return result
Argparse is a pretty object-oriented API, so I’m not going to test any of this bc/ that would be a chore. Sorry.
Are you/can you use MyPy? I’m a big fan of type-checkers in general, so if you’re using one that’s great! But (python being python) not everything I’ll suggest can necessarily work with MyPy.
Bullets:
- What does “Ronseal.” mean?
- The
descriptionargument toargparse.ArgumentParseris help text for the program, not a description of the argument parser. (doc) - If you’ve got a “main” method, just call it
main. - Why is
add_boolean_argumentsa separate function when the other args get added inmake_parser? - You could use dict unpacking for your calls to
add_argument. Exactly how to do this is hazy, particularly because (AFAIK) the “name” argument must be positional.ARGUMENTScould itself be a dict, with the names as keys. Or you could be fancy and have a dataclass, and put all the details of instantiating the CLI arg into a method of that dataclass. I make no argument that the version below is “good”. - Since all the attribute names align, you could use dict unpacking again to get the
HMSoftwareInstaller. It’s tempting to let argparse populate the object for us by passingnamespace=toparse_args, but we’d have to already have an object to do that…
import argparse
from dataclasses import dataclass, asdict
from functools import reduce
from typing import Any, Optional
# Local imports.
from git_credentials import set_up_git_credentials
from hm_software_installer import ...
@dataclass
class Argument:
name: str
default: Any
dest: str
help: str
action: Optional[str] = None # Could be an enum instead of str
type: type # I don't think the name collision is a problem
def __post_init__(self):
"""This is totally optional, and there may be a better way"""
assert isinstance(self.default, self.type)
@staticmethod # just because the args would be in the wrong order for `reduce`.
def add_to(parser, arg):
_arg = asdict(arg)
if "store_true" == arg.action:
_arg.pop("type") # This is ugly
parser.add_argument(_arg.pop("name"), **_arg)
return parser
ARGUMENTS = [Argument(name="--os",
default=DEFAULT_OS,
dest="this_os",
help="A string giving the OS in use on this system",
type=str),
...,
Argument(name="--reset-git-credentials-only",
action="store_true",
default=False,
dest="reset_git_credentials_only",
help="Reset the Git credentials, but perform no installations",
type=bool)]
def make_parser():
# Using `reduce` is kinda silly, because `add_argument` mutates the parser in-place.
return reduce(Argument.add_to,
ARGUMENTS,
argparse.ArgumentParser(description="..."))
def make_installer_obj(arguments):
return HMSoftwareInstaller(**arguments)
def run_git_credentials_function(git_username,
email_address,
path_to_git_credentials,
path_to_pat,
**__): # ignore any other arguments.
set_up_git_credentials(username=git_username,
email_address=email_address,
path_to_git_credentials=path_to_git_credentials,
path_to_pat=path_to_pat
def main():
""" Run this file. """
parser = make_parser()
arguments = vars(parser.parse_args())
if arguments.pop("reset_git_credentials_only"):
run_git_credentials_function(**arguments)
else:
installer = make_installer_obj(arguments)
installer.run()
if __name__ == "__main__":
main()
```
I’ve never seen anyone use a dictionary for argparse before, and as such it throws me somewhat off. What is reasonably standard for larger sets of arguments is to have these defined in a separate function (like your make_parser one). You’re kind of limiting yourself, as can be seen with the boolean argument; imagine how complicated it would become if you wanted to introduce mutually exclusive arguments, as you actually should be doing with the git authentication options.
You also seem to be using dest fairly extensively for no real reason – I’d typically only use it for snake_case conversion and for avoiding variables that otherwise match keywords and built-ins.
I’d also recommend rethinking having all of your default and config options be global variables – some of them are fine (e.g. default git config path), but others might be more suitable to deal with using, for example, configparser.
Separate, small comment, would be the formatting for multiple-line imports.
Most recommendations are to not use to extend lines, but group via an expression syntax, which will automatically extend over lines if not yet closed:
# not recommended:
from AModule import
a,
b,
c
# recommended:
from BModule import (
a,
b,
c
)