Command Line Parser for my VBA package manager written in Python

Question

As the title suggests, I'm creating a package manager for VBA which is modelled loosely on conda/ poetry, allowing you to install VBA modules potentially into "environments", but certainly with some kind of dependency resolution so you can keep things harmonious and up-to-date automatically.

At the moment I'm working on main.py which is the CLI entry point, and I've created a first draft of the viv install command:

viv.main

"""
Created on Thu Oct 21 14:50:38 2021
"""
from __future__ import annotations

import argparse
from dataclasses import dataclass
from dataclasses import field
from pathlib import Path
from typing import Callable
from typing import Generic
from typing import NewType
from typing import Optional
from typing import Sequence
from typing import TYPE_CHECKING
from typing import TypeVar

import git
import yaml
from bidict import bidict

from viv.git import temp_repo_from_url

if TYPE_CHECKING:
    from typing import TypeAlias

    LocalRepo: TypeAlias = git.Repo
    LocalFileSource: TypeAlias = Path
    LocalPkg: TypeAlias = LocalRepo | LocalFileSource
PackageAlias = NewType("PackageAlias", str)
TPkg = TypeVar("TPkg", bound="LocalPkg")


def parse_requirement(package: str) -> tuple[str, Optional[str]]:
    """
    Parses the optional requirement string from a
    <package_ref><relational_operator><requirement_body> string

    Parameters
    ----------
    package : str
        foo, foo=..., foo>=... etc.
        If the package_ref contains one of the <relational_operator>s:
            `=, <, >, <=, >=, ==, or !=`,
        then it must be further escaped in single quotes

    Returns
    -------
    tuple[str, Optional[str]]
        <package_ref>, <requirement> (i.e. <first_relational_operator><requirement_body>)

    """

    RELATIONAL_OPERATORS = ("=", "==", "<", "<=", ">=", ">", "!=")
    if package.startswith("'"):
        try:
            # this will capture escaped 'foo' with or without <requirement>
            _, package_ref, requirement = package.split("'", 2)
        except ValueError:  # not enough values to unpack - i.e. escaped string not found
            pass
        else:
            return package_ref, requirement or None
    # Now try and find which operator comes first,
    # as this is what we want to split on
    longest_requirements = ""
    best_package_ref = ""
    for operator in RELATIONAL_OPERATORS:
        try:
            package_ref, requirement_body = package.split(operator, 1)
        except ValueError:
            pass
        else:
            # Need to compare length of the sum, because otherwise
            # this would preferentially match splitting on a shorter operator
            if (len(requirement_body) + len(operator)) > len(longest_requirements):
                best_package_ref = package_ref
                longest_requirements = operator + requirement_body
    else:
        if best_package_ref:
            return best_package_ref, longest_requirements
        else:
            # assume bare package with no requirements and no escaping
            return package, None


@dataclass
class PackageDescriptor(Generic[TPkg]):
    friendly_name: str
    reference: TPkg


@dataclass
class VivSession:
    aliases: bidict[PackageAlias, str]
    _descriptor_cache: dict[str, PackageDescriptor[LocalPkg]] = field(
        default_factory=dict,
        init=False,
        repr=False,
    )

    def _register_descriptor(
        self,
        descriptor: PackageDescriptor[LocalPkg],
        package_ref: str,
        alias: Optional[PackageAlias] = None,
    ) -> None:
        self._descriptor_cache[package_ref] = descriptor
        if alias:
            self._descriptor_cache[alias] = descriptor

    def get_or_generate_descriptor(
        self,
        package_ref: str,
    ) -> PackageDescriptor[LocalPkg]:
        """
        Lookup function to return info about a repo that is required by resolver
        Will generate the info if it has not already
        package_ref can be 1 of the following
         1. Local directory (which must contain a viv.toml, or viv will report an error).
         2. Project or archive URL.
         3. Local Repo.
         4. A known alias to any of the above
        """

        def make_descriptor(
            local_ref: LocalPkg,
            alias: Optional[PackageAlias],
            backup_friendly_name: Callable[[], str],
        ) -> PackageDescriptor[LocalPkg]:
            # TODO: cache for resolved local paths in case of mixed input
            result = (
                PackageDescriptor(alias, local_ref)
                if alias
                else PackageDescriptor(backup_friendly_name(), local_ref)
            )
            self._register_descriptor(result, package_ref, alias)
            return result

        # Early return from cache
        try:
            return self._descriptor_cache[package_ref]
        except KeyError:
            pass
        # 1. Is it an alias? If so deref but keep that as the friendly name
        as_alias: Optional[PackageAlias] = PackageAlias(package_ref)
        try:
            package_ref = self.aliases[as_alias]
        except KeyError:
            # It isn't an alias, but it may still have an alias
            try:
                as_alias = self.aliases.inverse[package_ref]
            except KeyError:
                as_alias = None
        # 2. Is it a local dir, file or remote?
        as_local = Path(package_ref)
        if not as_local.exists():
            # Anything not local is assumed to be a remote url we can clone
            as_repo = temp_repo_from_url(package_ref)
            return make_descriptor(
                as_repo,
                as_alias,
                lambda: as_repo.remotes.origin.url.split(".git")[0].split("/")[-1],
            )
        elif as_local.is_file():
            return make_descriptor(
                as_local,
                as_alias,
                lambda: as_local.resolve().parent.name,  # resolve so we can get the parent
            )
        else:
            return make_descriptor(
                git.Repo(as_local),
                as_alias,
                lambda: as_local.resolve().name,  # resolve so we can get name
            )


def main(argv: Optional[Sequence[str]] = None) -> int:
    """
    There are 4/5 formats of package we might get:
        - A bare remote url
        - A local repo path
        - Alias pointing to either of the above
        - A set of local files
    The manifest for installation/resolving is the combination of these with
    any existing packages installed (in the same env)
    """

    def parse_alias_map(arg: str) -> dict[str, str]:
        data = yaml.safe_load("{" + arg + "}")
        assert isinstance(data, dict), "A dict-like arg must be passed"
        return {PackageAlias(k): str(v) for k, v in data.items()}

    parser = argparse.ArgumentParser()
    subparsers = parser.add_subparsers(dest="command", required=True)

    install_parser = subparsers.add_parser("install", help="do the install step")
    install_parser.add_argument(
        "packages",
        help="package constraint pairs to be installed",
        type=str,
        nargs="+",  # ensure at least one thing to install
    )
    install_parser.add_argument(
        "-a",
        "--alias-map",
        help='dict like map e.g. "alias1: C:/local/repo/path, alias2: www.remote.url" (!!space after colons, use valid yaml syntax)',
        type=parse_alias_map,
        dest="aliases",
        default={},
    )

    args = parser.parse_args(argv)
    print(f"{argv=}")
    print(f"{args=}")
    assert args.command == "install", "Other commands not implemented yet"
    assert args.packages, "At least one package should have been passed"

    session = VivSession(aliases=bidict(args.aliases))
    print(f"{session=}")
    for package in args.packages:
        package_ref, requirement = parse_requirement(package)
        descriptor = session.get_or_generate_descriptor(package_ref)
        print(f"{descriptor=} {requirement=}")
    return 0


# %% main
if __name__ == "__main__":

    raise SystemExit(main())

... which references some 3rd party modules as well as another module in this package:

viv.git

"""
Created on Wed Feb  2 13:24:06 2022
"""
import tempfile

import git


def temp_repo_from_url(url: git.PathLike) -> git.Repo:
    dest = tempfile.TemporaryDirectory(prefix="viv_git_")
    result = git.Repo.clone_from(url, dest.name)
    result._tmpdir = (  # type:ignore
        dest  # when this goes out of scope, the tempdir will be cleaned up
    )
    return result

In brief, the code:

• Sets up an argparser to capture the list of packages to install and any requirements/constraints on those.
• Parses that list to split the package identifier (package_ref) from the requirement (see the parse_requirement function)
• For the list of package_refs, the code determines what type of reference the package is, and generates the PackageDescriptor data required by the dependency resolution step (not included here).
• Finally, I've got some dummy code to print out the parsed package descriptors alongside their respective requirement specification strings. In the real application this info will be passed to the dependency resolver then onto installation of the package.

---

To clarify a little that second last step, viv install can accept 4 kinds of package:

1. A git repo url containing some VBA code and a special metadata file. This repo will be cloned temporarily to the local machine so the contents can be accessed - also a list of versions of the package.
2. A local git repo - useful for developing a multi-version package locally without expensive cloning.
3. An alias - e.g. pip install black => pip install https://github.com/psf/black. These are passed as an alias map at the command line (which is parsed as a yaml dict, see 4th example) for a sort of debug mode. In future they will come from the internet somewhere, so a developer can "publish" a package with a certain alias pointing to the source code repo. Small steps!
4. A local folder containing a special metadata file. This is like pip install .. Again, useful for dev.

I'm thinking of adding a 5th option which is to clone a local repo rather than one online (mix of options 1 and 2). This is because viv needs to pull the latest versions of a repo and checkout different commits, so may leave the repo in an unexpected state if it's sharing with the user (2nd option).

---

Examples:

viv -h
viv install -h
viv install https://github.com/psf/black=21.11b1
viv install my_black=21.11b1 --alias-map "my_black: https://github.com/psf/black"
viv install C:/path/to/repo[.git]
viv install C:/path/to/folder/viv.toml #metadata file for local folder
viv install foo 'ba<>r'>=1.1,<5.* -a "foo: somewhere, ba<>r: somewhere else"

The escaping is pretty flexible but hopefully intuitive. For the alias map, yaml is used as it minimises the need for escaping. The version requirements/constraints are meant to be PEP440 compliant as under the hood I'm using poetry's dependency resolver, so may as well stick to prior art.

---

Feedback

I'm really interested in:

• UX; this tool is targeted at a wide VBA audience who are often business users/ not programmers. I know that's weird, why do they need a package manager, so really in all likelihood this will be used by programmers who just happen to be using VBA, however I want to make it as user friendly and easy as possible. Argparse gives great help strings IMO.
• Testing. I'm really not sure where to begin with this - I typically write a few "integration/unit tests" in the if __name__ == "__main__": part of each module, as automatic unit tests I find tiresome and slow my workflow. Also mypy really helps reduce the need for small unit tests ("compiler assisted refactoring"), so integration tests often suffice. However with all this very slow git cloning and I/O I really don't know where to begin testing this more thoroughly. Some advice on how you would tackle this without it becoming onerous would be helpful. I use pre-commit with black and mypy enforced on every commit.
• Bugs & Vulnerabilities. I think when installing random code from online with a package manager the user is already taking on a big risk. However I don't want something like weird repo names/aliases to be a vulnerability. Can you spot any (that exceed the natural risks of a package manager)? Bugs are related to the need for testing.

... or anything else you want to say:)

Answer 1

I think you can safely claim that you aren't a beginner anymore.

Imports

This style:

from typing import Callable
from typing import Generic
from typing import NewType
from typing import Optional
from typing import Sequence
from typing import TYPE_CHECKING
from typing import TypeVar

is kind of nasty. I've seen it recommended before by some linters but not others. isort by default does the opposite, and tuple-imports as in

from typing import Callable, Generic, ...

This is a matter of taste. You apparently prefer the line-separated style because it improves diffs, which is nominally true; but you can have your cake and eat it too:

from typing import (
    Callable,
    Generic,
    NewType,
    ...

Entry point

I hope that when you say viv.main you actually mean viv.__main__, which is a built-in package structure better suited to main entry.

Separate away your argparse definitions from your main method.

Logic-by-exception

Pythonistas variously claim that logic-by-exception is either endemic to the language or standard practice. I find it gross and avoidable. So things like this:

        try:
            # this will capture escaped 'foo' with or without <requirement>
            _, package_ref, requirement = package.split("'", 2)
        except ValueError:  # not enough values to unpack - i.e. escaped string not found
            pass
        else:
            return package_ref, requirement or None

are better expressed as

# this will capture escaped 'foo' with or without <requirement>
parts = package.split("'", 2)
if len(parts) == 3:
    _, package_ref, requirement = parts
    return package_ref, requirement or None
# ...

Or if you want to be even fancier, read about PEP636 structural pattern matching. Similar story for your KeyError.

Command-line switch inference

I find this:

        package_ref can be 1 of the following
         1. Local directory (which must contain a viv.toml, or viv will report an error).
         2. Project or archive URL.
         3. Local Repo.
         4. A known alias to any of the above

to make life harder for everyone - the programmer and the user. This kind of clever attempt at inference kneecaps validation and requires heuristics. You're better off having four mutually exclusive command-line arguments. Then, if the user passes something that doesn't smell like a URL, you can error out before sending the request to the HTTP stack (among other things). You ask:

I hadn't even considered passing a flag for distinguishing package types - do you envisage one flag for every package, or only one flag for each kind and an array of packages on the command line (collect)?

It's up to you. I think a simple and understandable style would be e.g.

--url http://a --url http://b --local ~/me.git

Asserts

As in another review, don't assert in production code. Raise exceptions instead (and not AssertionError!)

First: the syntax sucks. Lots of discussion - PEP679 included - partially covers this. Python made a massive and to my mind inexplicable mistake, when they were designing Python 3: they correctly required parens on print, and for whatever blessed reason, didn't similarly require parens on assert. assert syntax has some really nasty pitfalls that will silently do the wrong thing and ruin your day.

Second: as already mentioned in the other review, assert can be trivially disabled on the command line, and it should not be possible to disable your sanity checks. If you want to guard against programmer errors, great: do that in a unit test instead, which is more thorough, powerful and well-suited to the task. This is related to a pattern - that can become an anti-pattern if you're not careful - seen more commonly in C-likes: tailoring the program at compile time with the use of precompiler macros to a given environment or program flavour. This creates a complexity explosion - the programmer needs to understand not only the runtime state of the program, but the compile-time state of the program, and that can complicate debugging and deployment.

Third: intent matters. The intent of an assert, as communicated to future self and other programmers, is that of a non-production test. This is a matter of convention and industrially shared understanding.

Built-in exit

Don't raise SystemExit where you've done it - instead, just exit().

You say:

raise SystemExit is done because apparently if you run python with the -S flag or if site modules are not present, exit won't exist without explicit import whereas raise SystemExit should always work - explanation on YouTube here

Ehhhhhhhh. See the command-line docs and the site docs for further reading. I've not once seen an environment where this has been omitted. This is such a narrow and unlikely technicality that in my opinion the cure is worse than the disease.

If you are deeply concerned about this edge case, from sys import exit is a far more reasonable approach than raising the exception yourself.

Context management

Use TemporaryDirectory as a context manager.

Answer 2

Your assertion

assert args.command == "install", "Other commands not implemented yet"

can never run, as setting required=True on the associated subparser has already ensured this assertion will be met. Here is the error if I pass "a" instead of install.

 error: argument command: invalid choice: 'a' (choose from 'install')

This is also true for your assertion that at least one package was passed:

install: error: the following arguments are required: packages

Also note that assertions can be disabled with the -O flag. What this means is that you probably shouldn't be making assertions about conditions from which progress is impossible, and should instead raise something like a ValueError or even argparse.ArgumentError.