Deepen: argument parsing as an option table, and break the CliError exit-policy circularity #173

Recommendation strength: Worth exploring

Modules

src/messpy/cli.py — run, _parse_arguments and its dozen helpers, _dispatch_value_option, _apply_boolean_option, the VALUE_OPTIONS / BOOLEAN_OPTIONS / REPORT_FORMATS constants, CliError, ExitPolicy.

Problem

Two separate frictions at the same seam.

The dispatch is code where it should be data. _dispatch_value_option and _apply_boolean_option select on the option name through chained branches ending in an else fallthrough. An option added to VALUE_OPTIONS but missed in the dispatch does not fail loudly — it lands in the final branch. The option tables and the code that acts on them are two statements of the same list, kept in agreement by hand.

CliError carries exit policy, and run does not trust it. The error type has a policy flag bolted on:

class CliError(Exception):
    def __init__(self, message: str, ignore_errors_on_exit: bool = False) -> None:
        super().__init__(message)
        self.ignore_errors_on_exit = ignore_errors_on_exit

That flag must be threaded through roughly a dozen raise sites, each of which has to know the exit policy in order to report a parse error — even though the policy is itself something parsing produces. The circularity shows up at the top:

        if (parsed_arguments and parsed_arguments.exit_policy.ignore_errors_on_exit) or getattr(
            error, "ignore_errors_on_exit", False
        ):

run uses getattr with a default against an exception type it defines and fully controls. That is the code telling you the invariant is not actually guaranteed — a CliError may or may not carry the flag, and the top-level handler has to hedge.

Proposed deepening

Make the option set a table of descriptors — name, kind, how to apply it, how to validate it — and have _parse_arguments walk the table rather than branch on names. Adding an option becomes one row. The else fallthrough disappears because an unknown option is no longer representable.

Separate the two things CliError currently conflates. Parse the exit policy first, from the raw argv, as its own small module; then let failures carry only a message and let run consult the already-known policy. The dozen raise sites stop needing to know about exit behaviour, the flag comes off the exception, and the getattr hedge in run becomes a plain attribute read — or vanishes.

Deletion test: the option table concentrates option knowledge in one place, so yes. The CliError change is smaller — it removes an accidental dependency rather than concentrating a body of behaviour — which is part of why this is Worth exploring rather than Strong, and why it could reasonably be split into two tickets.

Benefits

Locality. Everything about an option — its spelling, its kind, its validation, its diagnostic — sits in one row instead of being spread across a frozenset and a branch chain. Exit-policy knowledge sits in one place instead of at a dozen raise sites.

Leverage. Adding a CLI option becomes a data edit. The diagnostics for unknown options, missing values and bad values can be generated from the table, so they stay consistent for free.

Test impact

Argument parsing is ~195 lines across _parse_arguments and twelve helpers, and it is targeted by exactly one test (with eight sub-cases), which asserts assertIn(diagnostic, stderr.getvalue()). Every other parsing behaviour is covered only incidentally, by tests that happened to pass the right flags for some other purpose.

With an option table, the diagnostic matrix — unknown option, missing value, repeated option, invalid format name, boolean option given a value — becomes a parameterised table test, which is both cheaper to write and far more complete than the single test that exists. These still drive real argv through real parsing; no mocks are involved.

Splitting exit-policy parsing out makes the exit-code matrix testable against argv directly, rather than only through a full analysis run that must be arranged to produce findings or errors on demand. The end-to-end exit-status matrix test should stay — it is proving composition, which is exactly what an end-to-end test is for.