From 57999a3be4966fdd4605af77d6bfd48c96e274ae Mon Sep 17 00:00:00 2001 From: erseco Date: Tue, 7 Jul 2026 23:41:23 +0100 Subject: [PATCH 1/2] Add --fields support for machine-readable CLI output Add a reusable field-selection helper to the CLI output layer and wire a --fields option into the four migrated list commands (courses, categories, sections, users). - select_fields(rows, fields): projects each row to the requested top-level keys, preserving the user-provided order, and raises a typed UnknownFieldError (naming the offending field and listing the available ones) when a field is absent from every row. - parse_fields(value): parses a comma-separated --fields value, treating an empty/whitespace-only value as 'no filtering' (equivalent to omitting it). - emit() gains an optional, backwards-compatible fields parameter applied to json/yaml/csv output; table output accepts but ignores it (best-effort). For CSV the selected keys become the column headers so all machine-readable formats share the same projected shape. Unknown fields exit non-zero with the error reported on stderr. --- src/py_moodle/cli/categories.py | 21 ++- src/py_moodle/cli/courses.py | 19 +- src/py_moodle/cli/output.py | 115 ++++++++++++ src/py_moodle/cli/sections.py | 21 ++- src/py_moodle/cli/users.py | 21 ++- tests/unit/test_cli_fields.py | 301 ++++++++++++++++++++++++++++++++ 6 files changed, 491 insertions(+), 7 deletions(-) create mode 100644 tests/unit/test_cli_fields.py diff --git a/src/py_moodle/cli/categories.py b/src/py_moodle/cli/categories.py index 5a20a4f..f5e333d 100644 --- a/src/py_moodle/cli/categories.py +++ b/src/py_moodle/cli/categories.py @@ -1,5 +1,7 @@ """Category management commands for ``py-moodle``.""" +from typing import Optional + import typer from rich.table import Table @@ -9,7 +11,7 @@ delete_category, list_categories, ) -from py_moodle.cli.output import OutputFormat, emit, get_console +from py_moodle.cli.output import OutputFormat, emit, get_console, parse_fields from py_moodle.session import MoodleSession app = typer.Typer(help="Manage course categories: list, create, delete.") @@ -41,6 +43,15 @@ def list_all_categories( "--output", help="Output format: table, json, yaml, or csv.", ), + fields: Optional[str] = typer.Option( + None, + "--fields", + help=( + "Comma-separated top-level fields to keep in machine-readable " + "output (json/yaml/csv), in the given order. Ignored for table " + "output; an unknown field is an error." + ), + ), ): """ Lists all available course categories. @@ -68,7 +79,13 @@ def _render_table(data): ) get_console(ctx).print(table) - emit(categories, output, table_fn=_render_table, csv_fields=_LIST_CSV_FIELDS) + emit( + categories, + output, + table_fn=_render_table, + csv_fields=_LIST_CSV_FIELDS, + fields=parse_fields(fields), + ) except MoodleCategoryError as e: typer.echo(f"Error listing categories: {e}", err=True) raise typer.Exit(1) diff --git a/src/py_moodle/cli/courses.py b/src/py_moodle/cli/courses.py index 94bb5bd..79f14d3 100644 --- a/src/py_moodle/cli/courses.py +++ b/src/py_moodle/cli/courses.py @@ -2,6 +2,7 @@ from dataclasses import asdict from functools import partial +from typing import Optional import typer from rich.table import Table @@ -10,6 +11,7 @@ OutputFormat, emit, get_console, + parse_fields, render_dry_run_plan, ) from py_moodle.course import ( @@ -54,6 +56,15 @@ def list_all_courses( "--output", help="Output format: table, json, yaml, or csv.", ), + fields: Optional[str] = typer.Option( + None, + "--fields", + help=( + "Comma-separated top-level fields to keep in machine-readable " + "output (json/yaml/csv), in the given order. Ignored for table " + "output; an unknown field is an error." + ), + ), ): """ Lists all available courses. @@ -75,7 +86,13 @@ def _render_table(data): ) get_console(ctx).print(table) - emit(courses, output, table_fn=_render_table, csv_fields=_LIST_CSV_FIELDS) + emit( + courses, + output, + table_fn=_render_table, + csv_fields=_LIST_CSV_FIELDS, + fields=parse_fields(fields), + ) def _print_course_summary_table(ctx: typer.Context, course_data: dict): diff --git a/src/py_moodle/cli/output.py b/src/py_moodle/cli/output.py index 6212056..d806fa3 100644 --- a/src/py_moodle/cli/output.py +++ b/src/py_moodle/cli/output.py @@ -23,6 +23,94 @@ LOGGER_NAME = "py_moodle" +class UnknownFieldError(ValueError): + """Raised when ``--fields`` names a field absent from every row. + + Attributes: + unknown: The requested field names that are not present in any row. + available: The field names actually present across the rows, in + first-seen order, offered to help the user recover. + """ + + def __init__(self, unknown: Sequence[str], available: Sequence[str]) -> None: + """Build a clear message naming the offending and available fields. + + Args: + unknown: The unrecognized field names. + available: The field names present across the rows. + """ + self.unknown = list(unknown) + self.available = list(available) + unknown_str = ", ".join(repr(field) for field in self.unknown) + available_str = ", ".join(self.available) or "(none)" + super().__init__( + f"Unknown field(s): {unknown_str}. Available fields: {available_str}" + ) + + +def parse_fields(value: Optional[str]) -> Optional[list]: + """Parse a comma-separated ``--fields`` value into a list of field names. + + An empty or whitespace-only value (e.g. ``--fields ""``) is treated as + "no filtering" and returns ``None``, exactly as if the flag were omitted. + + Args: + value: The raw ``--fields`` option value, or ``None`` when the flag + was not provided. + + Returns: + A list of non-empty, stripped field names preserving the + user-provided order, or ``None`` when no filtering should be applied. + """ + if value is None: + return None + fields = [part.strip() for part in value.split(",")] + fields = [part for part in fields if part] + return fields or None + + +def select_fields(rows: Sequence[Any], fields: Sequence[str]) -> list: + """Project each row to the requested fields, preserving field order. + + Only flat, top-level keys are supported (no nested/dotted selectors). + Each returned row is a new dict whose keys are exactly ``fields`` in the + requested order; a field present in some rows but missing from a given + row is filled with ``None`` so that all rows share a consistent shape. + + Args: + rows: A list of dict-like rows to project. An empty list is returned + unchanged (validation is skipped, since there are no keys to + validate against). + fields: The field names to keep, in the desired output order. + + Returns: + A new list of dicts containing only ``fields``, in order. + + Raises: + UnknownFieldError: If any requested field is not present in any row. + """ + if not rows: + return [] + + available: list = [] + seen: set = set() + for row in rows: + if isinstance(row, dict): + for key in row.keys(): + if key not in seen: + seen.add(key) + available.append(key) + + unknown = [field for field in fields if field not in seen] + if unknown: + raise UnknownFieldError(unknown, available) + + return [ + {field: (row.get(field) if isinstance(row, dict) else None) for field in fields} + for row in rows + ] + + class OutputFormat(str, enum.Enum): """Supported CLI output formats. @@ -103,6 +191,7 @@ def emit( output_format: OutputFormat, table_fn: Optional[Callable[[Any], None]] = None, csv_fields: Optional[Sequence[CsvField]] = None, + fields: Optional[Sequence[str]] = None, ) -> None: """Emit ``data`` in the requested output format. @@ -117,11 +206,34 @@ def emit( ``(header, accessor)`` pair, where ``accessor`` is either a dict key or a callable extracting the value from a row. When omitted, the union of keys present in ``data`` is used. + fields: Optional flat, top-level keys to keep in the machine-readable + output (JSON/YAML/CSV), in the given order. ``None`` (the default) + applies no filtering, preserving the existing behavior. For CSV, + the selected keys become the column headers (``csv_fields`` is + ignored) so that all formats share the same projected shape. For + ``TABLE`` output, ``fields`` is accepted but ignored (best-effort). Raises: ValueError: If ``output_format`` is ``TABLE`` and no ``table_fn`` is provided. + typer.Exit: If ``fields`` names a field absent from every row (the + offending field is reported on stderr with a non-zero exit code). """ + if fields is not None and output_format != OutputFormat.TABLE: + rows = data if isinstance(data, list) else [data] + try: + projected = select_fields(rows, fields) + except UnknownFieldError as exc: + typer.echo(str(exc), err=True) + raise typer.Exit(code=1) + if isinstance(data, list): + data = projected + else: + data = projected[0] if projected else {} + # The selected keys are the projected shape shared by all formats, so + # discard the pretty CSV column definitions in favor of those keys. + csv_fields = None + if output_format == OutputFormat.JSON: typer.echo(json.dumps(data, indent=2, ensure_ascii=False)) elif output_format == OutputFormat.YAML: @@ -225,7 +337,10 @@ def _render_table(data: dict) -> None: __all__ = [ "OutputFormat", + "UnknownFieldError", "emit", + "select_fields", + "parse_fields", "get_console", "configure_logging", "render_dry_run_plan", diff --git a/src/py_moodle/cli/sections.py b/src/py_moodle/cli/sections.py index ecfdb6c..d7db3ae 100644 --- a/src/py_moodle/cli/sections.py +++ b/src/py_moodle/cli/sections.py @@ -1,11 +1,13 @@ """Section management commands for ``py-moodle``.""" +from typing import Optional + import typer from rich.box import SQUARE from rich.table import Table # Import the new centralized function and corresponding error -from py_moodle.cli.output import OutputFormat, emit, get_console +from py_moodle.cli.output import OutputFormat, emit, get_console, parse_fields from py_moodle.course import MoodleCourseError, get_course_with_sections_and_modules # Keep the action functions (create/delete) that are still valid @@ -46,6 +48,15 @@ def list_course_sections( "--output", help="Output format: table, json, yaml, or csv.", ), + fields: Optional[str] = typer.Option( + None, + "--fields", + help=( + "Comma-separated top-level fields to keep in machine-readable " + "output (json/yaml/csv), in the given order. Ignored for table " + "output; an unknown field is an error." + ), + ), ): """ Lists a summary of all sections in a specific course. @@ -85,7 +96,13 @@ def _render_table(data): ) get_console(ctx).print(table) - emit(sections, output, table_fn=_render_table, csv_fields=_LIST_CSV_FIELDS) + emit( + sections, + output, + table_fn=_render_table, + csv_fields=_LIST_CSV_FIELDS, + fields=parse_fields(fields), + ) except MoodleCourseError as e: typer.echo(f"Error listing sections: {e}", err=True) diff --git a/src/py_moodle/cli/users.py b/src/py_moodle/cli/users.py index bfd6c44..05187cc 100644 --- a/src/py_moodle/cli/users.py +++ b/src/py_moodle/cli/users.py @@ -1,9 +1,11 @@ """User management commands for ``py-moodle``.""" +from typing import Optional + import typer from rich.table import Table -from py_moodle.cli.output import OutputFormat, emit, get_console +from py_moodle.cli.output import OutputFormat, emit, get_console, parse_fields from py_moodle.session import MoodleSession from py_moodle.user import MoodleUserError, create_user, delete_user, list_course_users @@ -35,6 +37,15 @@ def list_users_in_course( "--output", help="Output format: table, json, yaml, or csv.", ), + fields: Optional[str] = typer.Option( + None, + "--fields", + help=( + "Comma-separated top-level fields to keep in machine-readable " + "output (json/yaml/csv), in the given order. Ignored for table " + "output; an unknown field is an error." + ), + ), ): """Lists users enrolled in a specific course.""" ms = MoodleSession.get(ctx.obj["env"]) @@ -57,7 +68,13 @@ def _render_table(data): ) get_console(ctx).print(table) - emit(users, output, table_fn=_render_table, csv_fields=_LIST_CSV_FIELDS) + emit( + users, + output, + table_fn=_render_table, + csv_fields=_LIST_CSV_FIELDS, + fields=parse_fields(fields), + ) except MoodleUserError as e: typer.echo(f"Error listing users: {e}", err=True) raise typer.Exit(1) diff --git a/tests/unit/test_cli_fields.py b/tests/unit/test_cli_fields.py new file mode 100644 index 0000000..053806e --- /dev/null +++ b/tests/unit/test_cli_fields.py @@ -0,0 +1,301 @@ +"""Unit tests for issue #66: ``--fields`` machine-readable field selection. + +These tests cover the reusable ``select_fields``/``parse_fields`` helpers in +``py_moodle.cli.output`` and the ``--fields`` option wired into the four +migrated ``list`` commands (courses, categories, sections, users). All tests +run without any network access, Docker, or a live Moodle instance, mirroring +the mocking style of ``tests/unit/test_cli_output_formats.py``. +""" + +from __future__ import annotations + +import csv +import io +import json + +import pytest +from typer.testing import CliRunner + +from py_moodle.cli.output import ( + UnknownFieldError, + parse_fields, + select_fields, +) + + +def _cli_runner_with_separate_stderr() -> CliRunner: + """Build a CliRunner with stdout/stderr captured separately. + + Click <8.2 requires ``mix_stderr=False`` for this; Click >=8.2 removed + that argument and always captures stderr separately. Support both. + """ + try: + return CliRunner(mix_stderr=False) + except TypeError: + return CliRunner() + + +# Per-command invocation config for the parametrized CLI tests. Each entry +# describes how to mock the command's dependencies and what data it returns. +_COMMAND_CONFIGS = { + "courses": { + "module": "py_moodle.cli.courses", + "list_attr": "list_courses", + "base_args": ["courses", "list"], + "rows": [ + { + "id": 1, + "shortname": "cs101", + "fullname": "Intro to CS", + "categoryid": 3, + "visible": 1, + }, + { + "id": 2, + "shortname": "cs102", + "fullname": "Data Structures", + "categoryid": 3, + "visible": 0, + }, + ], + }, + "categories": { + "module": "py_moodle.cli.categories", + "list_attr": "list_categories", + "base_args": ["categories", "list"], + "rows": [ + {"id": 1, "name": "Science", "parent": 0, "coursecount": 5}, + {"id": 2, "name": "Maths", "parent": 1, "coursecount": 2}, + ], + }, + "users": { + "module": "py_moodle.cli.users", + "list_attr": "list_course_users", + "base_args": ["users", "list", "--course-id", "1"], + "rows": [ + {"id": 1, "fullname": "Ada Lovelace", "email": "ada@example.test"}, + {"id": 2, "fullname": "Alan Turing", "email": "alan@example.test"}, + ], + }, + "sections": { + "module": "py_moodle.cli.sections", + "list_attr": None, # sections list uses get_course_with_sections_and_modules + "base_args": ["sections", "list", "--course-id", "1"], + "rows": [ + {"id": 10, "section": 0, "name": "General", "modules": [], "visible": 1}, + {"id": 11, "section": 1, "name": "Week 1", "modules": [], "visible": 1}, + ], + }, +} + + +def _patch_command(monkeypatch, key): + """Monkeypatch the session and data source for one list command. + + Args: + monkeypatch: The pytest ``monkeypatch`` fixture. + key: The command key in :data:`_COMMAND_CONFIGS`. + + Returns: + The base CLI argument list for invoking that command. + """ + from unittest.mock import MagicMock + + config = _COMMAND_CONFIGS[key] + module = config["module"] + rows = config["rows"] + + monkeypatch.setattr(f"{module}.MoodleSession.get", lambda env=None: MagicMock()) + + if key == "sections": + monkeypatch.setattr( + f"{module}.get_course_with_sections_and_modules", + lambda *a, **k: {"fullname": "Course", "sections": rows}, + ) + else: + monkeypatch.setattr(f"{module}.{config['list_attr']}", lambda *a, **k: rows) + + return list(config["base_args"]) + + +# --------------------------------------------------------------------------- +# Unit tests: select_fields / parse_fields +# --------------------------------------------------------------------------- + + +def test_select_fields_returns_requested_keys_in_order(): + """select_fields projects each row to exactly the requested keys, in order.""" + rows = [ + {"id": 1, "shortname": "cs101", "fullname": "A"}, + {"id": 2, "shortname": "cs102", "fullname": "B"}, + ] + + result = select_fields(rows, ["shortname", "id"]) + + assert result == [ + {"shortname": "cs101", "id": 1}, + {"shortname": "cs102", "id": 2}, + ] + # Key order must follow the user-provided field order. + assert [list(row.keys()) for row in result] == [["shortname", "id"]] * 2 + + +def test_select_fields_unknown_field_raises_typed_error(): + """select_fields raises UnknownFieldError naming the offending field(s).""" + rows = [{"id": 1, "shortname": "cs101"}] + + with pytest.raises(UnknownFieldError) as excinfo: + select_fields(rows, ["id", "bogus"]) + + assert "bogus" in str(excinfo.value) + assert excinfo.value.unknown == ["bogus"] + # The available fields are surfaced to help the user recover. + assert "shortname" in str(excinfo.value) + + +def test_select_fields_empty_rows_returns_empty_without_validation(): + """An empty input yields an empty result and no unknown-field error.""" + assert select_fields([], ["anything"]) == [] + + +def test_select_fields_fills_missing_key_with_none_when_present_elsewhere(): + """A field present in some rows is kept for all rows (None when absent).""" + rows = [{"id": 1, "email": "a@b.test"}, {"id": 2}] + + result = select_fields(rows, ["id", "email"]) + + assert result == [{"id": 1, "email": "a@b.test"}, {"id": 2, "email": None}] + + +def test_parse_fields_splits_and_strips(): + """parse_fields splits on commas and strips surrounding whitespace.""" + assert parse_fields("id, shortname ,fullname") == ["id", "shortname", "fullname"] + + +def test_parse_fields_empty_is_treated_as_no_filtering(): + """Empty / whitespace-only --fields values mean 'no filtering' (None).""" + assert parse_fields(None) is None + assert parse_fields("") is None + assert parse_fields(" ") is None + assert parse_fields(",, ,") is None + + +# --------------------------------------------------------------------------- +# CLI tests: --fields on the list commands +# --------------------------------------------------------------------------- + + +def test_courses_list_json_fields_selects_keys_in_order(monkeypatch): + """`courses list --output json --fields shortname,id` selects those keys, ordered.""" + from py_moodle.cli.app import app + + args = _patch_command(monkeypatch, "courses") + + runner = CliRunner() + result = runner.invoke(app, args + ["--output", "json", "--fields", "shortname,id"]) + + assert result.exit_code == 0 + payload = json.loads(result.output) + assert payload == [ + {"shortname": "cs101", "id": 1}, + {"shortname": "cs102", "id": 2}, + ] + assert [list(obj.keys()) for obj in payload] == [["shortname", "id"]] * 2 + + +def test_courses_list_csv_fields_emits_selected_columns_in_order(monkeypatch): + """`courses list --output csv --fields id,shortname` emits those columns, ordered.""" + from py_moodle.cli.app import app + + args = _patch_command(monkeypatch, "courses") + + runner = CliRunner() + result = runner.invoke(app, args + ["--output", "csv", "--fields", "id,shortname"]) + + assert result.exit_code == 0 + rows = list(csv.reader(io.StringIO(result.output))) + assert rows[0] == ["id", "shortname"] + assert rows[1] == ["1", "cs101"] + assert rows[2] == ["2", "cs102"] + + +def test_courses_list_unknown_field_fails_with_stderr_message(monkeypatch): + """An unknown --fields value exits non-zero and names the field on stderr.""" + from py_moodle.cli.app import app + + args = _patch_command(monkeypatch, "courses") + + runner = _cli_runner_with_separate_stderr() + result = runner.invoke(app, args + ["--output", "json", "--fields", "id,bogus"]) + + assert result.exit_code != 0 + assert result.stdout == "" + assert "bogus" in result.stderr + + +def test_courses_list_empty_fields_leaves_output_unchanged(monkeypatch): + """An empty --fields value behaves exactly like omitting the flag.""" + from py_moodle.cli.app import app + + args = _patch_command(monkeypatch, "courses") + rows = _COMMAND_CONFIGS["courses"]["rows"] + + runner = CliRunner() + with_flag = runner.invoke(app, args + ["--output", "json", "--fields", ""]) + without_flag = runner.invoke(app, args + ["--output", "json"]) + + assert with_flag.exit_code == 0 + assert without_flag.exit_code == 0 + assert json.loads(with_flag.output) == rows + assert json.loads(without_flag.output) == rows + + +def test_courses_list_table_ignores_fields(monkeypatch): + """For --output table, --fields is accepted but ignored (best-effort).""" + from py_moodle.cli.app import app + + args = _patch_command(monkeypatch, "courses") + + runner = CliRunner() + result = runner.invoke(app, args + ["--fields", "id"]) + + assert result.exit_code == 0 + # The full table is rendered regardless of --fields; unselected columns + # such as "Fullname" are still present. + assert "Fullname" in result.output + + +@pytest.mark.parametrize("key", ["courses", "categories", "sections", "users"]) +def test_each_list_command_applies_fields_for_json(monkeypatch, key): + """Every migrated list command honors --fields for json output.""" + from py_moodle.cli.app import app + + args = _patch_command(monkeypatch, key) + + runner = CliRunner() + result = runner.invoke(app, args + ["--output", "json", "--fields", "id"]) + + assert result.exit_code == 0 + payload = json.loads(result.output) + assert payload + assert all(list(obj.keys()) == ["id"] for obj in payload) + + +@pytest.mark.parametrize( + "subcmd", + [ + ["courses", "list", "--help"], + ["categories", "list", "--help"], + ["sections", "list", "--help"], + ["users", "list", "--help"], + ], +) +def test_each_list_command_advertises_fields_option(subcmd): + """Each migrated list command advertises --fields in its --help output.""" + from py_moodle.cli.app import app + + runner = CliRunner() + result = runner.invoke(app, subcmd) + + assert result.exit_code == 0 + assert "--fields" in result.output From 017ad33c50b67f2053d19050f0d4612ea0555eec Mon Sep 17 00:00:00 2001 From: erseco Date: Tue, 7 Jul 2026 23:51:22 +0100 Subject: [PATCH 2/2] Assert --fields option via Click introspection, not rendered help MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The help-advertisement test parsed rendered --help text, which the newer typer/rich in CI (typer 0.26 vs 0.23 locally) truncates in the options panel at an 80-column terminal, so the literal '--fields' string was absent even though the option is wired correctly (all functional --fields tests pass; only these 4 help-render assertions failed: 4 failed, 196 passed on CI). Introspect the underlying Click command's declared option strings instead — deterministic and width/version independent. --- tests/unit/test_cli_fields.py | 51 +++++++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/tests/unit/test_cli_fields.py b/tests/unit/test_cli_fields.py index 053806e..81fa7c8 100644 --- a/tests/unit/test_cli_fields.py +++ b/tests/unit/test_cli_fields.py @@ -281,21 +281,44 @@ def test_each_list_command_applies_fields_for_json(monkeypatch, key): assert all(list(obj.keys()) == ["id"] for obj in payload) +def _command_option_names(*path: str) -> list[str]: + """Return the declared option strings of a CLI subcommand. + + Introspects the underlying Click command tree rather than parsing + rendered ``--help`` text, which is fragile across typer/rich versions + (the help panel truncates long content at narrow terminal widths). + + Args: + *path: The subcommand path, e.g. ``"courses", "list"``. + + Returns: + Every option string (e.g. ``"--fields"``) declared on the command. + """ + import click + import typer + + from py_moodle.cli.app import app + + command: click.Command = typer.main.get_command(app) + ctx = click.Context(command) + for name in path: + command = command.get_command(ctx, name) + assert command is not None, f"subcommand {name!r} not found" + names: list[str] = [] + for param in command.params: + names.extend(getattr(param, "opts", [])) + return names + + @pytest.mark.parametrize( - "subcmd", + "path", [ - ["courses", "list", "--help"], - ["categories", "list", "--help"], - ["sections", "list", "--help"], - ["users", "list", "--help"], + ["courses", "list"], + ["categories", "list"], + ["sections", "list"], + ["users", "list"], ], ) -def test_each_list_command_advertises_fields_option(subcmd): - """Each migrated list command advertises --fields in its --help output.""" - from py_moodle.cli.app import app - - runner = CliRunner() - result = runner.invoke(app, subcmd) - - assert result.exit_code == 0 - assert "--fields" in result.output +def test_each_list_command_advertises_fields_option(path): + """Each migrated list command declares a --fields option.""" + assert "--fields" in _command_option_names(*path)