From 139925e660e7f3be1dc14454b1f0be7f72cfa1f0 Mon Sep 17 00:00:00 2001 From: ak2k <19240940+ak2k@users.noreply.github.com> Date: Sat, 16 May 2026 22:11:38 -0400 Subject: [PATCH 1/3] Provider namespacing: generic --providers/--cash-only/--awards-only (work-4byx) Replaces the per-provider PP flag block (--no-pp/--pp-only/--pp-airlines/ --pp-cabin) with a generic provider-namespaced surface that scales to N providers. Lands before work-2eoa (Seats.aero) so that issue ships as a registry addition + a config.toml section, not eight new flags. New surface on `flight search`: --providers pp[,seats] CSV of providers to use (default: all enabled) --cash-only Skip every award provider --awards-only Skip cash table; show award providers only --provider-opt KEY=VAL Repeatable; e.g. pp.airlines=United,Delta Per-provider tuning moves out of the CLI into ~/.config/flight-cli/config.toml ([providers.] tables). CLI --provider-opt overrides config.toml values for a single invocation. Deprecated aliases retained for one release (hidden=True so --help is clean; runtime warning surfaces when set). Conflicts between old and new flags raise a clear error rather than silently picking one. Pyright-clean. 29 new tests across test_config.py, test_provider_selection.py, and test_registry.py covering loader, CLI parsing, alias forwarding, conflict detection, config<-CLI merge precedence, registry filter matching. --- src/flight_cli/_config.py | 102 ++++++++++ src/flight_cli/cli.py | 285 +++++++++++++++++++++++---- src/flight_cli/pp/cli.py | 2 + src/flight_cli/providers/registry.py | 21 +- tests/test_config.py | 118 +++++++++++ tests/test_provider_selection.py | 226 +++++++++++++++++++++ tests/test_registry.py | 23 ++- 7 files changed, 736 insertions(+), 41 deletions(-) create mode 100644 src/flight_cli/_config.py create mode 100644 tests/test_config.py create mode 100644 tests/test_provider_selection.py diff --git a/src/flight_cli/_config.py b/src/flight_cli/_config.py new file mode 100644 index 0000000..e440109 --- /dev/null +++ b/src/flight_cli/_config.py @@ -0,0 +1,102 @@ +"""User-level config loader for ~/.config/flight-cli/config.toml. + +Today's only consumer is the provider section — per-provider airline lists, +cabin lists, etc. The shape is intentionally flat per provider so adding a +new provider (seats.aero, etc.) is a new `[providers.]` table, not a +schema migration. + +The loader is forgiving: missing file → empty config, partial sections → +fill in with what's there, unknown keys → kept verbatim (callers decide +whether to honor them). +""" + +from __future__ import annotations + +import os +import tomllib +from pathlib import Path +from typing import Any + +CONFIG_DIR_ENV = "FLIGHT_CLI_CONFIG_DIR" +DEFAULT_CONFIG_PATH = Path.home() / ".config" / "flight-cli" / "config.toml" + + +def _config_path() -> Path: + override = os.environ.get(CONFIG_DIR_ENV) + if override: + return Path(override) / "config.toml" + return DEFAULT_CONFIG_PATH + + +def load() -> dict[str, Any]: + """Read config.toml, return parsed dict. Missing file → {}. + + Parse errors raise — silent fallback would hide typos that defeat the + user's intent.""" + path = _config_path() + if not path.exists(): + return {} + with path.open("rb") as f: + return tomllib.load(f) + + +def provider_options(name: str, *, config: dict[str, Any] | None = None) -> dict[str, Any]: + """Pull the `[providers.]` table out of config, or {} if absent. + + Caller is responsible for type-coercing the values (lists stay as lists, + strings stay as strings — TOML's native typing is preserved).""" + cfg = config if config is not None else load() + providers = cfg.get("providers", {}) + if not isinstance(providers, dict): + return {} + section = providers.get(name, {}) + if not isinstance(section, dict): + return {} + return section + + +def parse_provider_opt_overrides(raw: list[str]) -> dict[str, dict[str, Any]]: + """Parse `--provider-opt pp.airlines=United,Delta` items into nested dict. + + `pp.airlines=United,Delta` → `{"pp": {"airlines": ["United", "Delta"]}}`. + A bare scalar (no comma) stays a string; comma-list becomes list[str]. + + Repeated keys for the same provider merge at the outer dict; same inner + key wins last (caller is responsible for order if it matters). + """ + out: dict[str, dict[str, Any]] = {} + for item in raw: + if "=" not in item: + msg = f"--provider-opt {item!r} missing '='; expected 'provider.key=value'" + raise ValueError(msg) + path, value = item.split("=", 1) + if "." not in path: + msg = f"--provider-opt {item!r} missing '.'; expected 'provider.key=value'" + raise ValueError(msg) + provider, key = path.split(".", 1) + provider = provider.strip() + key = key.strip() + if not provider or not key: + msg = f"--provider-opt {item!r}: provider and key must be non-empty" + raise ValueError(msg) + parsed: str | list[str] = ( + [v.strip() for v in value.split(",")] if "," in value else value.strip() + ) + out.setdefault(provider, {})[key] = parsed + return out + + +def merge_provider_options( + base: dict[str, Any], override: dict[str, Any] +) -> dict[str, Any]: + """Shallow merge: override's keys win, both at provider level and key level. + + Used to layer CLI --provider-opt on top of config.toml.""" + out: dict[str, Any] = {**base} + for k, v in override.items(): + existing = out.get(k) + if isinstance(existing, dict) and isinstance(v, dict): + out[k] = {**existing, **v} + else: + out[k] = v + return out diff --git a/src/flight_cli/cli.py b/src/flight_cli/cli.py index 35ea2ac..bf111ee 100644 --- a/src/flight_cli/cli.py +++ b/src/flight_cli/cli.py @@ -24,6 +24,7 @@ from rich.console import Console from rich.table import Table +from . import _config from .client import MatrixApiError, MatrixClient from .domain import ( Cabin, @@ -272,6 +273,171 @@ def _should_run_pp(*, no_pp: bool, pp_only: bool) -> bool: return True +class ProviderSelection: + """Resolved provider selection: what runs, which subset, with what options. + + `provider_filter` is a tuple of provider names to use (None = all enabled). + `cash_only` skips every award provider. `awards_only` suppresses the cash + table render. `provider_opts` is `{provider_name: {key: value}}` — e.g. + `{"pp": {"airlines": ["United", "Delta"], "cabins": ["Economy"]}}`. + """ + + __slots__ = ("awards_only", "cash_only", "provider_filter", "provider_opts") + + def __init__( + self, + *, + provider_filter: tuple[str, ...] | None, + cash_only: bool, + awards_only: bool, + provider_opts: dict[str, dict[str, Any]], + ) -> None: + self.provider_filter = provider_filter + self.cash_only = cash_only + self.awards_only = awards_only + self.provider_opts = provider_opts + + def pp_airlines(self) -> str | None: + """Backward-compat shim: PP's `airlines` as CSV (None = use default).""" + v = self.provider_opts.get("pp", {}).get("airlines") + if v is None: + return None + if isinstance(v, list): + return ",".join(str(x) for x in v) + return str(v) + + def pp_cabins(self) -> str | None: + """Backward-compat shim: PP's `cabins` as CSV (None = use default).""" + v = self.provider_opts.get("pp", {}).get("cabins") + if v is None: + return None + if isinstance(v, list): + return ",".join(str(x) for x in v) + return str(v) + + +def _resolve_providers( + *, + providers: str | None, + cash_only: bool, + awards_only: bool, + provider_opt: tuple[str, ...], + # deprecated aliases — forwarded into the new shape: + legacy_no_pp: bool = False, + legacy_pp_only: bool = False, + legacy_pp_airlines: str | None = None, + legacy_pp_cabin: str | None = None, +) -> ProviderSelection: + """Resolve the new + deprecated provider flags into one ProviderSelection. + + Precedence for per-provider options: config.toml < --provider-opt CLI. + Deprecated --pp-airlines / --pp-cabin map onto the --provider-opt path + so legacy invocations land in the same downstream shape. + """ + # Conflict checks across the new surface. + if cash_only and awards_only: + err.print("[red]--cash-only and --awards-only are mutually exclusive.[/]") + raise typer.Exit(2) + # Conflict checks across the old + new surface. + if legacy_no_pp and (cash_only or awards_only or providers is not None): + err.print("[red]--no-pp conflicts with the new --cash-only/--awards-only/--providers surface; use one.[/]") + raise typer.Exit(2) + if legacy_pp_only and (cash_only or awards_only or providers is not None): + err.print("[red]--pp-only conflicts with the new --cash-only/--awards-only/--providers surface; use one.[/]") + raise typer.Exit(2) + if legacy_no_pp and legacy_pp_only: + err.print("[red]--no-pp and --pp-only are mutually exclusive.[/]") + raise typer.Exit(2) + + # Forward legacy intent. + if legacy_no_pp: + cash_only = True + if legacy_pp_only: + awards_only = True + + # --providers parsing. + provider_filter: tuple[str, ...] | None = None + if providers is not None: + provider_filter = tuple(p.strip() for p in providers.split(",") if p.strip()) + if not provider_filter: + err.print("[red]--providers cannot be empty.[/]") + raise typer.Exit(2) + + # Build provider_opts: config.toml < --provider-opt CLI. + try: + config = _config.load() + except (OSError, ValueError) as e: + err.print(f"[red]Failed to load ~/.config/flight-cli/config.toml: {e}[/]") + raise typer.Exit(2) from e + base_opts: dict[str, dict[str, Any]] = {} + providers_section = config.get("providers", {}) if isinstance(config, dict) else {} + if isinstance(providers_section, dict): + for name, opts in providers_section.items(): + if isinstance(opts, dict): + base_opts[name] = dict(opts) + + try: + cli_opts = _config.parse_provider_opt_overrides(list(provider_opt)) + except ValueError as e: + err.print(f"[red]{e}[/]") + raise typer.Exit(2) from e + merged_opts = _config.merge_provider_options(base_opts, cli_opts) + + # Forward legacy --pp-airlines / --pp-cabin into the merged opts. + # CLI --provider-opt still wins (no-op if user set both, since merged_opts + # already has the override from cli_opts). + pp_section: dict[str, Any] = dict(merged_opts.get("pp", {})) + if legacy_pp_airlines is not None and "airlines" not in pp_section: + pp_section["airlines"] = [ + v.strip() for v in legacy_pp_airlines.split(",") if v.strip() + ] + if legacy_pp_cabin is not None and "cabins" not in pp_section: + pp_section["cabins"] = [ + v.strip() for v in legacy_pp_cabin.split(",") if v.strip() + ] + if pp_section: + merged_opts["pp"] = pp_section + + return ProviderSelection( + provider_filter=provider_filter, + cash_only=cash_only, + awards_only=awards_only, + provider_opts=merged_opts, + ) + + +def _should_run_awards(sel: ProviderSelection) -> bool: + """Award providers run iff (a) not --cash-only, (b) at least one is + configured, and (c) the filter (if any) names at least one configured + provider. Mirrors the old _should_run_pp semantics for the PP-only era; + generalizes naturally once more providers exist. + """ + if sel.cash_only: + return False + # Today we only know about PP. has_any_configured() is provider-blind by + # construction (see providers/registry.py) so this stays correct when + # work-2eoa adds seats.aero. + from .providers.registry import has_any_configured # noqa: PLC0415 + + if not has_any_configured(): + if sel.awards_only: + err.print( + "[red]--awards-only set but no award provider is configured.[/] " + "Run `flight auth pp login --tokens-file ...` first.", + ) + raise typer.Exit(2) + return False + if sel.provider_filter is not None and "pp" not in sel.provider_filter: + # Filter excludes PP; nothing else is configured yet. + if sel.awards_only: + err.print( + f"[red]--awards-only set but --providers={sel.provider_filter} matches no configured provider.[/]", + ) + raise typer.Exit(2) + return False + return True + + # ─────────────────────────── shared execution ────────────────────────────── @@ -504,9 +670,7 @@ def _run_matrix_path( matrix_url: bool, google_url: bool, run_pp: bool, - pp_only: bool, - pp_airlines: str | None, - pp_cabin: str | None, + sel: ProviderSelection, ) -> None: """Matrix path: Alkali call → optional cash render → optional PP augmentation → URLs.""" search = SpecificDateSearch(legs=legs, options=opts) @@ -515,7 +679,7 @@ def _run_matrix_path( if json_out and not run_pp: sys.stdout.write(json.dumps(res.raw, indent=2)) return - if not pp_only: + if not sel.awards_only: _render_search(res) if run_pp: p = opts.pax @@ -523,10 +687,11 @@ def _run_matrix_path( res, legs=_build_pp_legs(legs), num_passengers=p.adults + p.children + p.seniors + p.youth, - airlines=pp_airlines, - cabins=pp_cabin, - pp_only=pp_only, + airlines=sel.pp_airlines(), + cabins=sel.pp_cabins(), + pp_only=sel.awards_only, json_out=json_out, + provider_filter=sel.provider_filter, ) _emit_urls(search, matrix_url=matrix_url, google_url=google_url) @@ -538,9 +703,7 @@ def _run_gflight_path( top_n: int, json_out: bool, run_pp: bool = False, - pp_only: bool = False, - pp_airlines: str | None = None, - pp_cabin: str | None = None, + sel: ProviderSelection | None = None, ) -> None: """Google Flights path: build fli filter → query → render. Single-leg or round-trip. @@ -583,7 +746,8 @@ def _run_gflight_path( sys.stdout.write(json.dumps(out, indent=2, default=str)) return - if not pp_only: + awards_only = sel.awards_only if sel is not None else False + if not awards_only: _render_gflight_table(results, legs=legs, top_n=top_n) if run_pp: @@ -595,10 +759,11 @@ def _run_gflight_path( sr, legs=_build_pp_legs(legs), num_passengers=p.adults + p.children + p.seniors + p.youth, - airlines=pp_airlines, - cabins=pp_cabin, - pp_only=pp_only, + airlines=sel.pp_airlines() if sel is not None else None, + cabins=sel.pp_cabins() if sel is not None else None, + pp_only=awards_only, json_out=json_out, + provider_filter=sel.provider_filter if sel is not None else None, ) @@ -817,31 +982,55 @@ def search( matrix_url: bool = typer.Option(True, "--matrix-url/--no-matrix-url"), google_url: bool = typer.Option(True, "--google-url/--no-google-url"), no_cache: bool = typer.Option(False, "--no-cache"), - no_pp: bool = typer.Option( + providers: str | None = typer.Option( + None, + "--providers", + help=( + "CSV of award providers to use (e.g. 'pp'). " + "Default: all configured providers." + ), + ), + cash_only: bool = typer.Option( False, - "--no-pp", + "--cash-only", + help="Skip all award providers; just show the cash table.", + ), + awards_only: bool = typer.Option( + False, + "--awards-only", + help="Skip the cash table; show only the award provider output.", + ), + provider_opt: list[str] = typer.Option( + [], + "--provider-opt", help=( - "Skip PointsPath award augmentation even if tokens are present. " - "PP runs implicitly on matrix backend when valid tokens exist." + "Per-provider override, repeatable: 'pp.airlines=United,Delta'. " + "Overrides ~/.config/flight-cli/config.toml [providers.]." ), ), + no_pp: bool = typer.Option( + False, + "--no-pp", + hidden=True, + help="[deprecated] Use --cash-only.", + ), pp_only: bool = typer.Option( False, "--pp-only", - help="Show only PointsPath award availability; skip Matrix cash table.", + hidden=True, + help="[deprecated] Use --awards-only.", ), pp_airlines: str | None = typer.Option( None, "--pp-airlines", - help=( - "CSV of PointsPath airline names (e.g. United,Delta). " - "Default: discovered from your account's enabled airline set." - ), + hidden=True, + help="[deprecated] Use --provider-opt pp.airlines=A,B.", ), pp_cabin: str | None = typer.Option( None, "--pp-cabin", - help="CSV of cabins to query (Economy,Business,First). Default: Economy,Business.", + hidden=True, + help="[deprecated] Use --provider-opt pp.cabins=Economy,Business.", ), ) -> None: """Specific-date flight search across Matrix and Google Flights backends. @@ -850,6 +1039,23 @@ def search( Matrix-only flag is set (routing/extension/multi-city slice/time-of-day/ extra pax types/PP config). Force with --backend matrix|gflight. """ + # Deprecated-flag warning surfaces at runtime since hidden=True hides the + # banner from --help. + if any(x for x in (no_pp, pp_only, pp_airlines, pp_cabin)): + err.print( + "[yellow]--no-pp/--pp-only/--pp-airlines/--pp-cabin are deprecated; " + "use --cash-only / --awards-only / --provider-opt instead.[/]", + ) + sel = _resolve_providers( + providers=providers, + cash_only=cash_only, + awards_only=awards_only, + provider_opt=tuple(provider_opt), + legacy_no_pp=no_pp, + legacy_pp_only=pp_only, + legacy_pp_airlines=pp_airlines, + legacy_pp_cabin=pp_cabin, + ) resolved = _pick_backend( backend=backend, routing=routing, @@ -906,21 +1112,18 @@ def search( page_size=page_size, ) + run_awards = _should_run_awards(sel) if resolved == BACKEND_GFLIGHT: - run_pp = _should_run_pp(no_pp=no_pp, pp_only=pp_only) _run_gflight_path( legs=legs, opts=opts, top_n=page_size, json_out=json_out, - run_pp=run_pp, - pp_only=pp_only, - pp_airlines=pp_airlines, - pp_cabin=pp_cabin, + run_pp=run_awards, + sel=sel, ) return - run_pp = _should_run_pp(no_pp=no_pp, pp_only=pp_only) _run_matrix_path( legs=legs, opts=opts, @@ -930,10 +1133,8 @@ def search( json_out=json_out, matrix_url=matrix_url, google_url=google_url, - run_pp=run_pp, - pp_only=pp_only, - pp_airlines=pp_airlines, - pp_cabin=pp_cabin, + run_pp=run_awards, + sel=sel, ) @@ -1080,7 +1281,17 @@ def fare( show_only_available=only_available, page_size=page_size, ) - run_pp = _should_run_pp(no_pp=no_pp, pp_only=pp_only) + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=(), + legacy_no_pp=no_pp, + legacy_pp_only=pp_only, + legacy_pp_airlines=pp_airlines, + legacy_pp_cabin=pp_cabin, + ) + run_pp = _should_run_awards(sel) _run_matrix_path( legs=legs, opts=opts, @@ -1091,9 +1302,7 @@ def fare( matrix_url=matrix_url, google_url=google_url, run_pp=run_pp, - pp_only=pp_only, - pp_airlines=pp_airlines, - pp_cabin=pp_cabin, + sel=sel, ) diff --git a/src/flight_cli/pp/cli.py b/src/flight_cli/pp/cli.py index e49625b..d7010b4 100644 --- a/src/flight_cli/pp/cli.py +++ b/src/flight_cli/pp/cli.py @@ -195,6 +195,7 @@ def run_pp_for_search( cabins: str | None = None, pp_only: bool = False, json_out: bool = False, + provider_filter: tuple[str, ...] | None = None, ) -> None: """Run award augmentation through the provider registry, join against `res`'s cash itineraries, render. Today the registry hands back @@ -227,6 +228,7 @@ async def _go() -> tuple[list[list[AwardFlight]], list[Any]]: cabins=cabin_list, pp_airlines=explicit_airlines, cash_hints_per_leg=cash_hints_per_leg, + provider_filter=provider_filter, ) try: diff --git a/src/flight_cli/providers/registry.py b/src/flight_cli/providers/registry.py index d7bb6a3..21e9082 100644 --- a/src/flight_cli/providers/registry.py +++ b/src/flight_cli/providers/registry.py @@ -31,6 +31,7 @@ async def _construct_enabled( *, pp_airlines: tuple[str, ...] | None = None, + provider_filter: tuple[str, ...] | None = None, ) -> list[AwardProvider]: """Build the list of enabled provider instances. @@ -38,9 +39,16 @@ async def _construct_enabled( configured providers get instantiated (which is when network/auth actually happens). Failures during construction are logged and swallowed so one provider's outage can't take down the others. + + `provider_filter` (when non-None) restricts to a named subset. Matching + is case-insensitive against the provider's short name (e.g. "pp", + "seats"). A filter that names no configured providers yields an empty + list — the caller decides whether that's a hard error (--awards-only) + or silent skip (cash-only path). """ out: list[AwardProvider] = [] - if pp_is_configured(): + allow_pp = provider_filter is None or _matches(provider_filter, "pp") + if allow_pp and pp_is_configured(): try: out.append(await PointsPathProvider.create(explicit_airlines=pp_airlines)) except Exception as e: # noqa: BLE001 — per-provider failures are non-fatal @@ -48,6 +56,11 @@ async def _construct_enabled( return out +def _matches(filter_: tuple[str, ...], name: str) -> bool: + """Case-insensitive membership test for the provider filter.""" + return any(p.strip().lower() == name.lower() for p in filter_) + + async def _gather_one_leg( providers: list[AwardProvider], leg: LegQuery, @@ -90,6 +103,7 @@ async def gather_awards( num_passengers: int = 1, pp_airlines: tuple[str, ...] | None = None, cash_hints_per_leg: list[tuple[CashFlightHint, ...]] | None = None, + provider_filter: tuple[str, ...] | None = None, ) -> tuple[list[list[AwardFlight]], list[AwardProvider]]: """End-to-end registry call: construct enabled providers, fan out per leg. @@ -106,7 +120,10 @@ async def gather_awards( providers is the constructed provider instances — the caller is responsible for closing them (PointsPath uses HTTP keepalive). """ - providers = await _construct_enabled(pp_airlines=pp_airlines) + providers = await _construct_enabled( + pp_airlines=pp_airlines, + provider_filter=provider_filter, + ) per_leg: list[list[AwardFlight]] = [] for i, leg in enumerate(legs): hints: tuple[CashFlightHint, ...] = () diff --git a/tests/test_config.py b/tests/test_config.py new file mode 100644 index 0000000..4710c21 --- /dev/null +++ b/tests/test_config.py @@ -0,0 +1,118 @@ +# pyright: reportPrivateUsage=false +"""Tests for the user-level config.toml loader. + +The loader powers the new --providers / --provider-opt surface (work-4byx) +and is the seed of the broader surface-hygiene config story (work-4uls). +Round-trip tests pin: missing file → {}, partial sections → preserved verbatim, +provider lookups by name, CLI override parsing, merge semantics. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import TYPE_CHECKING + +import pytest + +from flight_cli import _config + +if TYPE_CHECKING: + from pytest import MonkeyPatch + + +@pytest.fixture +def _config_dir(tmp_path: Path, monkeypatch: MonkeyPatch) -> Path: + monkeypatch.setenv(_config.CONFIG_DIR_ENV, str(tmp_path)) + return tmp_path + + +def test_load_missing_file_returns_empty(_config_dir: Path) -> None: + assert _config.load() == {} + + +def test_load_round_trip(_config_dir: Path) -> None: + (_config_dir / "config.toml").write_text( + """\ +[providers.pp] +airlines = ["United", "Delta"] +cabins = ["Economy", "Business"] + +[providers.seats] +api_key = "secret" +""" + ) + cfg = _config.load() + assert cfg["providers"]["pp"]["airlines"] == ["United", "Delta"] + assert cfg["providers"]["pp"]["cabins"] == ["Economy", "Business"] + assert cfg["providers"]["seats"]["api_key"] == "secret" + + +def test_provider_options_named_lookup(_config_dir: Path) -> None: + (_config_dir / "config.toml").write_text( + """\ +[providers.pp] +airlines = ["United"] +""" + ) + assert _config.provider_options("pp") == {"airlines": ["United"]} + assert _config.provider_options("seats") == {} + + +def test_provider_options_missing_providers_section(_config_dir: Path) -> None: + (_config_dir / "config.toml").write_text( + '[http]\nrps = 2.0\n' + ) + assert _config.provider_options("pp") == {} + + +def test_parse_provider_opt_csv_value() -> None: + parsed = _config.parse_provider_opt_overrides( + ["pp.airlines=United,Delta"], + ) + assert parsed == {"pp": {"airlines": ["United", "Delta"]}} + + +def test_parse_provider_opt_scalar_value() -> None: + parsed = _config.parse_provider_opt_overrides(["pp.api_key=secret"]) + assert parsed == {"pp": {"api_key": "secret"}} + + +def test_parse_provider_opt_multiple_providers() -> None: + parsed = _config.parse_provider_opt_overrides( + ["pp.airlines=United", "seats.api_key=k"], + ) + assert parsed == {"pp": {"airlines": "United"}, "seats": {"api_key": "k"}} + + +def test_parse_provider_opt_rejects_missing_eq() -> None: + with pytest.raises(ValueError, match="missing '='"): + _config.parse_provider_opt_overrides(["pp.airlines"]) + + +def test_parse_provider_opt_rejects_missing_dot() -> None: + with pytest.raises(ValueError, match="missing '.'"): + _config.parse_provider_opt_overrides(["airlines=United"]) + + +def test_parse_provider_opt_rejects_empty_provider() -> None: + with pytest.raises(ValueError, match="must be non-empty"): + _config.parse_provider_opt_overrides([".airlines=United"]) + + +def test_merge_provider_options_inner_keys() -> None: + base = {"pp": {"airlines": ["A"], "cabins": ["Economy"]}} + override = {"pp": {"airlines": ["B", "C"]}} + merged = _config.merge_provider_options(base, override) + assert merged == { + "pp": {"airlines": ["B", "C"], "cabins": ["Economy"]}, + } + + +def test_merge_provider_options_adds_new_provider() -> None: + base = {"pp": {"airlines": ["A"]}} + override = {"seats": {"api_key": "k"}} + merged = _config.merge_provider_options(base, override) + assert merged == { + "pp": {"airlines": ["A"]}, + "seats": {"api_key": "k"}, + } diff --git a/tests/test_provider_selection.py b/tests/test_provider_selection.py new file mode 100644 index 0000000..77305d4 --- /dev/null +++ b/tests/test_provider_selection.py @@ -0,0 +1,226 @@ +# pyright: reportPrivateUsage=false +"""Tests for the provider-selection resolver in cli.py. + +Covers: + - New surface: --providers / --cash-only / --awards-only / --provider-opt + - Deprecated alias forwarding: --no-pp / --pp-only / --pp-airlines / --pp-cabin + - Conflict detection across old + new surface + - Config.toml merging with CLI overrides +""" + +from __future__ import annotations + +from pathlib import Path +from typing import TYPE_CHECKING + +import pytest +import typer + +from flight_cli import _config +from flight_cli.cli import _resolve_providers + +if TYPE_CHECKING: + from pytest import MonkeyPatch + + +@pytest.fixture +def _config_dir(tmp_path: Path, monkeypatch: MonkeyPatch) -> Path: + monkeypatch.setenv(_config.CONFIG_DIR_ENV, str(tmp_path)) + return tmp_path + + +def test_default_resolution_no_flags(_config_dir: Path) -> None: + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=(), + ) + assert sel.provider_filter is None + assert sel.cash_only is False + assert sel.awards_only is False + assert sel.provider_opts == {} + + +def test_providers_csv_parsing(_config_dir: Path) -> None: + sel = _resolve_providers( + providers="pp,seats", + cash_only=False, + awards_only=False, + provider_opt=(), + ) + assert sel.provider_filter == ("pp", "seats") + + +def test_providers_empty_string_rejected(_config_dir: Path) -> None: + with pytest.raises(typer.Exit): + _resolve_providers( + providers=",,", + cash_only=False, + awards_only=False, + provider_opt=(), + ) + + +def test_cash_only_and_awards_only_conflict(_config_dir: Path) -> None: + with pytest.raises(typer.Exit): + _resolve_providers( + providers=None, + cash_only=True, + awards_only=True, + provider_opt=(), + ) + + +def test_legacy_no_pp_forwards_to_cash_only(_config_dir: Path) -> None: + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=(), + legacy_no_pp=True, + ) + assert sel.cash_only is True + + +def test_legacy_pp_only_forwards_to_awards_only(_config_dir: Path) -> None: + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=(), + legacy_pp_only=True, + ) + assert sel.awards_only is True + + +def test_legacy_pp_airlines_forwards_to_provider_opts(_config_dir: Path) -> None: + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=(), + legacy_pp_airlines="United,Delta", + ) + assert sel.provider_opts == {"pp": {"airlines": ["United", "Delta"]}} + assert sel.pp_airlines() == "United,Delta" + + +def test_legacy_pp_cabin_forwards_to_provider_opts(_config_dir: Path) -> None: + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=(), + legacy_pp_cabin="Economy,Business", + ) + assert sel.provider_opts == {"pp": {"cabins": ["Economy", "Business"]}} + assert sel.pp_cabins() == "Economy,Business" + + +def test_legacy_no_pp_conflicts_with_new_surface(_config_dir: Path) -> None: + with pytest.raises(typer.Exit): + _resolve_providers( + providers=None, + cash_only=True, + awards_only=False, + provider_opt=(), + legacy_no_pp=True, + ) + + +def test_legacy_pp_only_conflicts_with_providers(_config_dir: Path) -> None: + with pytest.raises(typer.Exit): + _resolve_providers( + providers="pp", + cash_only=False, + awards_only=False, + provider_opt=(), + legacy_pp_only=True, + ) + + +def test_provider_opt_cli_only(_config_dir: Path) -> None: + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=("pp.airlines=United,Delta",), + ) + assert sel.provider_opts == {"pp": {"airlines": ["United", "Delta"]}} + + +def test_config_toml_seeds_provider_opts(_config_dir: Path) -> None: + (_config_dir / "config.toml").write_text( + """\ +[providers.pp] +airlines = ["United"] +cabins = ["Economy"] +""" + ) + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=(), + ) + assert sel.provider_opts == {"pp": {"airlines": ["United"], "cabins": ["Economy"]}} + + +def test_cli_override_wins_over_config(_config_dir: Path) -> None: + (_config_dir / "config.toml").write_text( + '[providers.pp]\nairlines = ["United"]\n' + ) + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=("pp.airlines=Delta",), + ) + assert sel.provider_opts["pp"]["airlines"] == "Delta" + + +def test_cli_provider_opt_preempts_legacy_pp_airlines(_config_dir: Path) -> None: + """If both --provider-opt and --pp-airlines are set, --provider-opt wins. + + Rationale: the new surface is preferred; legacy is only for unmigrated + scripts. Erroring would break the migration path; silent wins-old would + surprise the user who reached for the new flag.""" + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=("pp.airlines=Delta",), + legacy_pp_airlines="United,JetBlue", + ) + assert sel.provider_opts["pp"]["airlines"] == "Delta" + + +def test_pp_airlines_helper_handles_list(_config_dir: Path) -> None: + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=("pp.airlines=United,Delta",), + ) + assert sel.pp_airlines() == "United,Delta" + + +def test_pp_cabins_helper_handles_scalar(_config_dir: Path) -> None: + sel = _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=("pp.cabins=Economy",), + ) + assert sel.pp_cabins() == "Economy" + + +def test_invalid_provider_opt_format(_config_dir: Path) -> None: + with pytest.raises(typer.Exit): + _resolve_providers( + providers=None, + cash_only=False, + awards_only=False, + provider_opt=("pp.airlines",), # missing = + ) diff --git a/tests/test_registry.py b/tests/test_registry.py index ba86c17..8bf6f0a 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -14,7 +14,7 @@ import pytest from flight_cli.providers.base import AwardFlight, AwardProvider, LegQuery -from flight_cli.providers.registry import _gather_one_leg +from flight_cli.providers.registry import _gather_one_leg, _matches if TYPE_CHECKING: from flight_cli.pp.client import CashFlightHint @@ -105,3 +105,24 @@ async def go() -> list[AwardFlight]: out: list[AwardFlight] = anyio.run(go) assert len(out) == count + + +# ─────────────────────────── provider filter (work-4byx) ───────────────── + + +def test_matches_is_case_insensitive() -> None: + assert _matches(("pp",), "PP") is True + assert _matches(("PP",), "pp") is True + assert _matches(("Pp",), "pp") is True + + +def test_matches_trims_whitespace() -> None: + assert _matches((" pp ",), "pp") is True + + +def test_matches_returns_false_for_unknown_provider() -> None: + assert _matches(("pp",), "seats") is False + + +def test_matches_empty_filter_returns_false() -> None: + assert _matches((), "pp") is False From 07e185a79a6ccb8b4cb9f8178702aa55d275d3af Mon Sep 17 00:00:00 2001 From: ak2k <19240940+ak2k@users.noreply.github.com> Date: Sat, 16 May 2026 22:30:52 -0400 Subject: [PATCH 2/3] flight-search skill: update flag reference for provider namespacing The skill listed --no-pp; replace with the new --cash-only / --awards-only / --providers / --provider-opt surface so agents construct invocations using the canonical flags. Deprecated aliases still work but the skill shouldn't teach them. --- .claude/skills/flight-search/SKILL.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.claude/skills/flight-search/SKILL.md b/.claude/skills/flight-search/SKILL.md index b70ef59..19cd409 100644 --- a/.claude/skills/flight-search/SKILL.md +++ b/.claude/skills/flight-search/SKILL.md @@ -26,7 +26,10 @@ Global flags (every search-printing command): - `--json` — emit raw response JSON - `--no-cache` — bypass the on-disk response cache - `--matrix-url` / `--google-url` — toggle deep-link emission -- `--no-pp` — skip the PointsPath award overlay even if tokens are logged in (PP runs implicitly on both matrix and gflight backends when tokens are present) +- `--cash-only` — skip all award providers; show only the cash table +- `--awards-only` — skip the cash table; show only the award provider output +- `--providers pp[,seats]` — restrict to a named subset of award providers (default: all configured) +- `--provider-opt KEY=VAL` — per-provider override, repeatable, e.g. `--provider-opt pp.airlines=United,Delta` or `--provider-opt pp.cabins=Economy,Business`. Defaults live in `~/.config/flight-cli/config.toml` under `[providers.]` tables. ## Intent → flag cheat sheet From 2232a7acc72e90737c4d7d18fd983e0913954d68 Mon Sep 17 00:00:00 2001 From: ak2k <19240940+ak2k@users.noreply.github.com> Date: Sun, 17 May 2026 00:25:06 -0400 Subject: [PATCH 3/3] fixup: lint + basedpyright fixes for CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - E501: split 3 long error-message lines under 100 chars - B008: extract --provider-opt typer.Option to module-level _PROVIDER_OPT (and make the default None, not [], handling None at the callsite) - RUF043: raw string for pytest.raises match=r"missing '\.'" - TC003: pathlib.Path moved into TYPE_CHECKING blocks (two test files) - PLR0912: noqa on _resolve_providers (single-purpose validator/merge) - reportUnknownVariableType: explicit Any annotations + cast around tomllib's dict parses; tightens the loader's pyright surface - reportUnusedFunction: pragma on _should_run_pp (still tested but no longer called from cli.py — slated for removal with the deprecated flag block); module-header for test fixtures whose use is via pytest parameter-name discovery - Replace `any(x for x in (...))` with plain `or` chain (pyright knows the types now) All in service of getting the work-4byx PR's CI green. No behavior change. --- src/flight_cli/_config.py | 17 ++++---- src/flight_cli/cli.py | 68 ++++++++++++++++---------------- tests/test_config.py | 11 +++--- tests/test_provider_selection.py | 9 ++--- 4 files changed, 51 insertions(+), 54 deletions(-) diff --git a/src/flight_cli/_config.py b/src/flight_cli/_config.py index e440109..d0e37c1 100644 --- a/src/flight_cli/_config.py +++ b/src/flight_cli/_config.py @@ -15,7 +15,7 @@ import os import tomllib from pathlib import Path -from typing import Any +from typing import Any, cast CONFIG_DIR_ENV = "FLIGHT_CLI_CONFIG_DIR" DEFAULT_CONFIG_PATH = Path.home() / ".config" / "flight-cli" / "config.toml" @@ -46,13 +46,14 @@ def provider_options(name: str, *, config: dict[str, Any] | None = None) -> dict Caller is responsible for type-coercing the values (lists stay as lists, strings stay as strings — TOML's native typing is preserved).""" cfg = config if config is not None else load() - providers = cfg.get("providers", {}) - if not isinstance(providers, dict): + providers_any: Any = cfg.get("providers", {}) + if not isinstance(providers_any, dict): return {} - section = providers.get(name, {}) - if not isinstance(section, dict): + providers_dict = cast("dict[str, Any]", providers_any) + section_any: Any = providers_dict.get(name, {}) + if not isinstance(section_any, dict): return {} - return section + return cast("dict[str, Any]", section_any) def parse_provider_opt_overrides(raw: list[str]) -> dict[str, dict[str, Any]]: @@ -86,9 +87,7 @@ def parse_provider_opt_overrides(raw: list[str]) -> dict[str, dict[str, Any]]: return out -def merge_provider_options( - base: dict[str, Any], override: dict[str, Any] -) -> dict[str, Any]: +def merge_provider_options(base: dict[str, Any], override: dict[str, Any]) -> dict[str, Any]: """Shallow merge: override's keys win, both at provider level and key level. Used to layer CLI --provider-opt on top of config.toml.""" diff --git a/src/flight_cli/cli.py b/src/flight_cli/cli.py index bf111ee..c44450c 100644 --- a/src/flight_cli/cli.py +++ b/src/flight_cli/cli.py @@ -246,7 +246,7 @@ def _pick_backend( return backend -def _should_run_pp(*, no_pp: bool, pp_only: bool) -> bool: +def _should_run_pp(*, no_pp: bool, pp_only: bool) -> bool: # pyright: ignore[reportUnusedFunction] """Decide whether PP augmentation runs. Tokens present → True (unless --no-pp). Both backends support PP overlay @@ -299,24 +299,24 @@ def __init__( def pp_airlines(self) -> str | None: """Backward-compat shim: PP's `airlines` as CSV (None = use default).""" - v = self.provider_opts.get("pp", {}).get("airlines") + v: Any = self.provider_opts.get("pp", {}).get("airlines") if v is None: return None if isinstance(v, list): - return ",".join(str(x) for x in v) + return ",".join(str(x) for x in cast("list[Any]", v)) return str(v) def pp_cabins(self) -> str | None: """Backward-compat shim: PP's `cabins` as CSV (None = use default).""" - v = self.provider_opts.get("pp", {}).get("cabins") + v: Any = self.provider_opts.get("pp", {}).get("cabins") if v is None: return None if isinstance(v, list): - return ",".join(str(x) for x in v) + return ",".join(str(x) for x in cast("list[Any]", v)) return str(v) -def _resolve_providers( +def _resolve_providers( # noqa: PLR0912 — single-purpose validator + merge; splitting hurts readability *, providers: str | None, cash_only: bool, @@ -339,11 +339,16 @@ def _resolve_providers( err.print("[red]--cash-only and --awards-only are mutually exclusive.[/]") raise typer.Exit(2) # Conflict checks across the old + new surface. - if legacy_no_pp and (cash_only or awards_only or providers is not None): - err.print("[red]--no-pp conflicts with the new --cash-only/--awards-only/--providers surface; use one.[/]") + new_surface_set = cash_only or awards_only or providers is not None + if legacy_no_pp and new_surface_set: + err.print( + "[red]--no-pp conflicts with --cash-only/--awards-only/--providers; use one.[/]", + ) raise typer.Exit(2) - if legacy_pp_only and (cash_only or awards_only or providers is not None): - err.print("[red]--pp-only conflicts with the new --cash-only/--awards-only/--providers surface; use one.[/]") + if legacy_pp_only and new_surface_set: + err.print( + "[red]--pp-only conflicts with --cash-only/--awards-only/--providers; use one.[/]", + ) raise typer.Exit(2) if legacy_no_pp and legacy_pp_only: err.print("[red]--no-pp and --pp-only are mutually exclusive.[/]") @@ -370,11 +375,11 @@ def _resolve_providers( err.print(f"[red]Failed to load ~/.config/flight-cli/config.toml: {e}[/]") raise typer.Exit(2) from e base_opts: dict[str, dict[str, Any]] = {} - providers_section = config.get("providers", {}) if isinstance(config, dict) else {} + providers_section: Any = config.get("providers", {}) if isinstance(providers_section, dict): - for name, opts in providers_section.items(): + for name, opts in cast("dict[str, Any]", providers_section).items(): if isinstance(opts, dict): - base_opts[name] = dict(opts) + base_opts[name] = dict(cast("dict[str, Any]", opts)) try: cli_opts = _config.parse_provider_opt_overrides(list(provider_opt)) @@ -388,13 +393,9 @@ def _resolve_providers( # already has the override from cli_opts). pp_section: dict[str, Any] = dict(merged_opts.get("pp", {})) if legacy_pp_airlines is not None and "airlines" not in pp_section: - pp_section["airlines"] = [ - v.strip() for v in legacy_pp_airlines.split(",") if v.strip() - ] + pp_section["airlines"] = [v.strip() for v in legacy_pp_airlines.split(",") if v.strip()] if legacy_pp_cabin is not None and "cabins" not in pp_section: - pp_section["cabins"] = [ - v.strip() for v in legacy_pp_cabin.split(",") if v.strip() - ] + pp_section["cabins"] = [v.strip() for v in legacy_pp_cabin.split(",") if v.strip()] if pp_section: merged_opts["pp"] = pp_section @@ -431,7 +432,8 @@ def _should_run_awards(sel: ProviderSelection) -> bool: # Filter excludes PP; nothing else is configured yet. if sel.awards_only: err.print( - f"[red]--awards-only set but --providers={sel.provider_filter} matches no configured provider.[/]", + f"[red]--awards-only set but --providers={sel.provider_filter} " + "matches no configured provider.[/]", ) raise typer.Exit(2) return False @@ -894,6 +896,14 @@ def _fmt_gflight_legroom(fli_legs: list[Any], amenities: list[Any]) -> str: # Common-args helpers — these reduce repetition across commands. _RPS_OPT = typer.Option(1.0, help="Requests per second") _IMPERSONATE_OPT = typer.Option("chrome", help="curl_cffi profile") +_PROVIDER_OPT = typer.Option( + None, + "--provider-opt", + help=( + "Per-provider override, repeatable: 'pp.airlines=United,Delta'. " + "Overrides ~/.config/flight-cli/config.toml [providers.]." + ), +) @app.command() @@ -985,10 +995,7 @@ def search( providers: str | None = typer.Option( None, "--providers", - help=( - "CSV of award providers to use (e.g. 'pp'). " - "Default: all configured providers." - ), + help=("CSV of award providers to use (e.g. 'pp'). Default: all configured providers."), ), cash_only: bool = typer.Option( False, @@ -1000,14 +1007,7 @@ def search( "--awards-only", help="Skip the cash table; show only the award provider output.", ), - provider_opt: list[str] = typer.Option( - [], - "--provider-opt", - help=( - "Per-provider override, repeatable: 'pp.airlines=United,Delta'. " - "Overrides ~/.config/flight-cli/config.toml [providers.]." - ), - ), + provider_opt: list[str] | None = _PROVIDER_OPT, no_pp: bool = typer.Option( False, "--no-pp", @@ -1041,7 +1041,7 @@ def search( """ # Deprecated-flag warning surfaces at runtime since hidden=True hides the # banner from --help. - if any(x for x in (no_pp, pp_only, pp_airlines, pp_cabin)): + if no_pp or pp_only or pp_airlines or pp_cabin: err.print( "[yellow]--no-pp/--pp-only/--pp-airlines/--pp-cabin are deprecated; " "use --cash-only / --awards-only / --provider-opt instead.[/]", @@ -1050,7 +1050,7 @@ def search( providers=providers, cash_only=cash_only, awards_only=awards_only, - provider_opt=tuple(provider_opt), + provider_opt=tuple(provider_opt or ()), legacy_no_pp=no_pp, legacy_pp_only=pp_only, legacy_pp_airlines=pp_airlines, diff --git a/tests/test_config.py b/tests/test_config.py index 4710c21..438c8a4 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,4 +1,4 @@ -# pyright: reportPrivateUsage=false +# pyright: reportPrivateUsage=false, reportUnusedFunction=false """Tests for the user-level config.toml loader. The loader powers the new --providers / --provider-opt surface (work-4byx) @@ -9,7 +9,6 @@ from __future__ import annotations -from pathlib import Path from typing import TYPE_CHECKING import pytest @@ -17,6 +16,8 @@ from flight_cli import _config if TYPE_CHECKING: + from pathlib import Path + from pytest import MonkeyPatch @@ -59,9 +60,7 @@ def test_provider_options_named_lookup(_config_dir: Path) -> None: def test_provider_options_missing_providers_section(_config_dir: Path) -> None: - (_config_dir / "config.toml").write_text( - '[http]\nrps = 2.0\n' - ) + (_config_dir / "config.toml").write_text("[http]\nrps = 2.0\n") assert _config.provider_options("pp") == {} @@ -90,7 +89,7 @@ def test_parse_provider_opt_rejects_missing_eq() -> None: def test_parse_provider_opt_rejects_missing_dot() -> None: - with pytest.raises(ValueError, match="missing '.'"): + with pytest.raises(ValueError, match=r"missing '\.'"): _config.parse_provider_opt_overrides(["airlines=United"]) diff --git a/tests/test_provider_selection.py b/tests/test_provider_selection.py index 77305d4..4ffde46 100644 --- a/tests/test_provider_selection.py +++ b/tests/test_provider_selection.py @@ -1,4 +1,4 @@ -# pyright: reportPrivateUsage=false +# pyright: reportPrivateUsage=false, reportUnusedFunction=false """Tests for the provider-selection resolver in cli.py. Covers: @@ -10,7 +10,6 @@ from __future__ import annotations -from pathlib import Path from typing import TYPE_CHECKING import pytest @@ -20,6 +19,8 @@ from flight_cli.cli import _resolve_providers if TYPE_CHECKING: + from pathlib import Path + from pytest import MonkeyPatch @@ -168,9 +169,7 @@ def test_config_toml_seeds_provider_opts(_config_dir: Path) -> None: def test_cli_override_wins_over_config(_config_dir: Path) -> None: - (_config_dir / "config.toml").write_text( - '[providers.pp]\nairlines = ["United"]\n' - ) + (_config_dir / "config.toml").write_text('[providers.pp]\nairlines = ["United"]\n') sel = _resolve_providers( providers=None, cash_only=False,