From e53d6e110695bb4ba5023160a6811d261ee82994 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 8 Jul 2026 23:17:04 +0200 Subject: [PATCH 1/4] Rebase issue-108 onto main; reconcile #108 extraction with main's refactors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit main independently landed this branch's bundled #95/#96/#97, refactored bump.py (new `_CrateManifestOutcome` flow) and publish.py (#73/#96), and re-split the bump tests into differently-named modules. This reconciles the #108 extraction onto main's current tree as a single squashed change (the original 22 commits are preserved on backup/issue-108-pre-rebase2-7df1480). Landed cleanly (all now under the 400-line guideline): - `lading/toml_coerce/` package (scalar/sequence/mapping coercers with structural pattern matching and numpy docstrings) + `config.py` (441->334) and `workspace/models.py` (576->194) consuming it via `functools.partial`. - `lading/workspace/_coercion.py` + `graph_build.py` (builders out of `models`); `cli_options.py` (cyclopts args out of `cli.py`, 449->389); `bump_docs.py` condensed (402->393). - `bump_manifests.py` re-derived against main's bump.py: the low-level manifest helpers, `_WORKSPACE_SELECTORS`, and `_BumpContext` move here; main's `_apply_crate_manifest_update`/`_CrateManifestOutcome` flow stays in `bump` and imports them. `bump.py` 552->427 (further trim is follow-up). Test reconciliation: main's bump-test split is authoritative; this drops the branch's parallel split modules and keeps main's plus the unique `test_toml_coerce.py`. Per the no-compat-alias convention (#164), `test_dependency_sections` now reads `_DEPENDENCY_SECTION_BY_KIND` from `bump_manifests`. Deferred: `publish.py` (741) extraction. main's #73/#96 refactored the extracted pipeline functions and its tests patch `publish._*` seams; extracting now requires rewriting many `monkeypatch.setattr` targets to `publish_pipeline` (no-compat), which needs a focused full-context pass. publish.py is taken from main unchanged; the orphaned `publish_pipeline.py` is removed. Validated: check-fmt, lint (10.00/10), typecheck, test (717 passed, 70 snapshots), markdownlint, nixie — all green. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/developers-guide.md | 33 ++ docs/users-guide.md | 16 +- lading/cli.py | 84 +--- lading/cli_options.py | 100 +++++ lading/commands/bump.py | 163 +------ lading/commands/bump_docs.py | 15 +- lading/commands/bump_manifests.py | 197 +++++++++ lading/config.py | 139 +----- lading/toml_coerce/__init__.py | 29 ++ lading/toml_coerce/_core.py | 28 ++ lading/toml_coerce/_mappings.py | 125 ++++++ lading/toml_coerce/_scalars.py | 120 ++++++ lading/toml_coerce/_sequences.py | 140 ++++++ lading/workspace/__init__.py | 6 +- lading/workspace/_coercion.py | 65 +++ lading/workspace/graph_build.py | 398 +++++++++++++++++ lading/workspace/models.py | 406 +----------------- tests/bdd/steps/test_bump_steps.py | 6 +- .../unit/__snapshots__/test_toml_coerce.ambr | 13 + tests/unit/test_cli.py | 4 +- tests/unit/test_dependency_sections.py | 4 +- tests/unit/test_toml_coerce.py | 281 ++++++++++++ .../unit/test_workspace_models_validation.py | 58 +-- 23 files changed, 1643 insertions(+), 787 deletions(-) create mode 100644 lading/cli_options.py create mode 100644 lading/commands/bump_manifests.py create mode 100644 lading/toml_coerce/__init__.py create mode 100644 lading/toml_coerce/_core.py create mode 100644 lading/toml_coerce/_mappings.py create mode 100644 lading/toml_coerce/_scalars.py create mode 100644 lading/toml_coerce/_sequences.py create mode 100644 lading/workspace/_coercion.py create mode 100644 lading/workspace/graph_build.py create mode 100644 tests/unit/__snapshots__/test_toml_coerce.ambr create mode 100644 tests/unit/test_toml_coerce.py diff --git a/docs/developers-guide.md b/docs/developers-guide.md index ef526b2b..ffa2eff6 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -216,6 +216,13 @@ execution. When `command_runner` is `None`, bump falls back to the default subprocess runner. Tests pass a runner explicitly so lockfile commands can be observed without invoking real Cargo processes. +Bump-time crate-set derivation is centralized in the bump context: the +`excluded` and `updated_crate_names` sets are computed exactly once in +`bump._initialize_bump_context` and threaded to downstream helpers such as +`_update_crate_manifest`. Helpers must consume the context sets rather than +re-deriving them per crate, which would make manifest processing quadratic in +workspace size. + Option defaulting is the command layer's responsibility, not the CLI adapter's. `cli.bump` forwards `rebuild_lockfiles` as the raw `bool | None` it received; the only resolution against `configuration.bump.rebuild_lockfiles` happens in @@ -709,6 +716,32 @@ a formatted message that includes all four values. Using a single function for message construction keeps the error format consistent across the packaging and publish phases and makes snapshot testing straightforward. +### Shared TOML coercion (`lading.toml_coerce`) + +`lading.toml_coerce` is the canonical home for the TOML scalar, sequence, and +mapping coercion helpers shared by `lading.config` and +`lading.workspace.models`. Each helper takes an `error` keyword naming the +`LadingError` subclass to raise, and both consumers bind their domain error +type once with `functools.partial` (`ConfigurationError` in `config`, +`WorkspaceModelError` in `models`); neither module re-declares a coercer. The +canonical error-message shape is +`{field} must be {expected}; received {type(value).__name__}.` and is pinned by +property and snapshot tests in `tests/unit/test_toml_coerce.py`. + +### Module size extractions (issue 108) + +To keep source files within the 400-line guideline, the following +responsibilities live in dedicated modules, imported by their original homes: + +- `lading.commands.bump_manifests` — per-manifest version and + dependency-section rewriting plus the `_BumpContext` construction contract + (imported by `bump`). +- `lading.workspace.graph_build` — builders converting `cargo metadata` + output into workspace models; the error-bound coercion helpers live in + `lading.workspace._coercion` (both imported by `workspace`). +- `lading.cli_options` — Cyclopts argument declarations and version-argument + validation (imported by `cli`). + ### In-process metrics (`lading.utils.metrics`) `lading.utils.metrics` is a process-local metrics accumulator. Counters are diff --git a/docs/users-guide.md b/docs/users-guide.md index 2ab72ce6..c077b8f7 100644 --- a/docs/users-guide.md +++ b/docs/users-guide.md @@ -86,15 +86,19 @@ lading bump 1.2.3 --dry-run After updating manifest versions, `lading bump` automatically refreshes any git-tracked `Cargo.lock` files (excluding those under `target/`) that have an -adjacent `Cargo.toml`. Refreshed lockfiles are listed in the command output -with a `(lockfile)` suffix. In dry-run mode the lockfiles are listed but not -modified. +adjacent `Cargo.toml`. The result header counts each changed category, e.g. +"N manifest(s)", "N documentation file(s)", "N readme file(s)", "N +lockfile(s)", joined with Oxford-comma grammar. In the per-file body list, +manifest paths carry no suffix; the other categories carry a parenthetical +suffix: `(documentation)` for Markdown docs whose TOML code fences were +updated, `(readme)` for crate READMEs adopted from the workspace README, and +`(lockfile)` for regenerated `Cargo.lock` files. In dry-run mode, every file +is listed, but none are modified. Where a member crate sets `readme.workspace = true`, `lading bump` also adopts the workspace `README.md` into that crate's directory and rewrites relative -Markdown links so they still resolve from the crate directory. Adopted README -files are listed in the command output with a `(readme)` suffix. In dry-run -mode the files are listed but not written. +Markdown links so they still resolve from the crate directory. Such adopted +READMEs appear in the output with the `(readme)` suffix described above. If `bump.documentation.globs` is configured, `lading` also searches those Markdown files for TOML code fences and updates version values that refer to diff --git a/lading/cli.py b/lading/cli.py index a38231a9..98398d1b 100644 --- a/lading/cli.py +++ b/lading/cli.py @@ -22,73 +22,30 @@ import importlib import logging import os -import re import sys import typing as typ from contextlib import contextmanager from pathlib import Path -from cyclopts import App, Parameter +from cyclopts import App from . import commands, config +from .cli_options import ( + WORKSPACE_ROOT_ENV_VAR, + WORKSPACE_ROOT_REQUIRED_MESSAGE, + AllowUnpublishedWorkspaceDepsFlag, + DryRunFlag, + ForbidDirtyFlag, + LiveFlag, + RebuildLockfilesFlag, + VersionArgument, + WorkspaceRootOption, +) from .runtime import CommandRunner, subprocess_runner from .utils import metrics, normalise_workspace_root from .workspace import WorkspaceGraph, WorkspaceModelError, load_workspace from .workspace import metadata as metadata_module -WORKSPACE_ROOT_ENV_VAR = "LADING_WORKSPACE_ROOT" -WORKSPACE_ROOT_REQUIRED_MESSAGE = "--workspace-root requires a value" -_WORKSPACE_PARAMETER = Parameter( - name="workspace-root", - env_var=WORKSPACE_ROOT_ENV_VAR, - help="Path to the Rust workspace root.", -) -WorkspaceRootOption = typ.Annotated[Path, _WORKSPACE_PARAMETER] - -_VERSION_PARAMETER = Parameter( - help="Target semantic version (e.g., 1.2.3) to set across workspace manifests.", -) -VersionArgument = typ.Annotated[str, _VERSION_PARAMETER] - -_DRY_RUN_PARAMETER = Parameter( - name="dry-run", - help="Preview manifest changes without writing files.", -) -DryRunFlag = typ.Annotated[bool, _DRY_RUN_PARAMETER] - -_REBUILD_LOCKFILES_PARAMETER = Parameter( - name="rebuild-lockfiles", - negative="no-rebuild-lockfiles", - help="Regenerate Cargo.lock files after manifest updates.", -) -RebuildLockfilesFlag = typ.Annotated[bool, _REBUILD_LOCKFILES_PARAMETER] - -_LIVE_PARAMETER = Parameter( - name="live", - help="Run cargo publish without --dry-run; default behaviour is dry-run.", -) -LiveFlag = typ.Annotated[bool, _LIVE_PARAMETER] - -_FORBID_DIRTY_PARAMETER = Parameter( - name="forbid-dirty", - help=("Require a clean working tree before running publish pre-flight checks."), -) -ForbidDirtyFlag = typ.Annotated[bool, _FORBID_DIRTY_PARAMETER] - -_ALLOW_UNPUBLISHED_WORKSPACE_DEPS_PARAMETER = Parameter( - name="allow-unpublished-workspace-deps", - help=( - "Dry-run only: downgrade cargo package failures caused by a sibling " - "workspace crate version not yet on crates.io to a warning when the " - "missing crate is part of the planned publish set and appears earlier " - "in publish order. Defaults to enabled in dry-run mode. Cannot be " - "combined with --live." - ), -) -AllowUnpublishedWorkspaceDepsFlag = typ.Annotated[ - bool | None, _ALLOW_UNPUBLISHED_WORKSPACE_DEPS_PARAMETER -] - LOG_LEVEL_ENV_VAR = "LADING_LOG_LEVEL" _DEFAULT_LOG_LEVEL = logging.INFO _LOG_FORMAT = "%(levelname)s: %(message)s" @@ -361,22 +318,6 @@ def _run_with_context( return runner(workspace_root, configuration, workspace_model, active_runner) -_VERSION_PATTERN = re.compile( - r"^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$" -) - - -def _validate_version_argument(version: str) -> None: - """Ensure ``version`` matches the semantic version pattern.""" - if not _VERSION_PATTERN.fullmatch(version): - message = ( - "Invalid version argument " - f"{version!r}. Expected semantic version in the form " - ".. with optional pre-release/build segments." - ) - raise SystemExit(message) - - @app.command def bump( version: VersionArgument, @@ -386,7 +327,6 @@ def bump( rebuild_lockfiles: RebuildLockfilesFlag | None = None, ) -> str: """Update workspace manifests to ``version``.""" - _validate_version_argument(version) resolved = normalise_workspace_root(workspace_root) return _run_with_context( resolved, diff --git a/lading/cli_options.py b/lading/cli_options.py new file mode 100644 index 00000000..d532a499 --- /dev/null +++ b/lading/cli_options.py @@ -0,0 +1,100 @@ +"""Cyclopts argument declarations for the :mod:`lading` CLI. + +Extracted from :mod:`lading.cli` (issue #108) so option declarations live +apart from dispatch logic. ``cli`` re-imports every public name, so external +access through ``lading.cli`` keeps working. +""" + +from __future__ import annotations + +import re +import typing as typ +from pathlib import Path + +from cyclopts import Parameter + +WORKSPACE_ROOT_ENV_VAR = "LADING_WORKSPACE_ROOT" +WORKSPACE_ROOT_REQUIRED_MESSAGE = "--workspace-root requires a value" +_WORKSPACE_PARAMETER = Parameter( + name="workspace-root", + env_var=WORKSPACE_ROOT_ENV_VAR, + help="Path to the Rust workspace root.", +) +WorkspaceRootOption = typ.Annotated[Path, _WORKSPACE_PARAMETER] + +_VERSION_PATTERN = re.compile( + r"^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$" +) + + +def _validate_version_argument(_hint: object, version: str) -> None: + """Reject a non-semantic-version ``version`` via cyclopts' validator hook.""" + # Cyclopts calls validators as ``validator(type_hint, value)`` and converts a + # raised ValueError into a formatted ValidationError, so a bad version is + # reported through cyclopts' own error flow rather than a bare SystemExit. + if not _VERSION_PATTERN.fullmatch(version): + message = ( + "Invalid version argument " + f"{version!r}. Expected semantic version in the form " + ".. with optional pre-release/build segments." + ) + raise ValueError(message) + + +_VERSION_PARAMETER = Parameter( + validator=_validate_version_argument, + help="Target semantic version (e.g., 1.2.3) to set across workspace manifests.", +) +VersionArgument = typ.Annotated[str, _VERSION_PARAMETER] + +_DRY_RUN_PARAMETER = Parameter( + name="dry-run", + help="Preview manifest changes without writing files.", +) +DryRunFlag = typ.Annotated[bool, _DRY_RUN_PARAMETER] + +_REBUILD_LOCKFILES_PARAMETER = Parameter( + name="rebuild-lockfiles", + negative="no-rebuild-lockfiles", + help="Regenerate Cargo.lock files after manifest updates.", +) +RebuildLockfilesFlag = typ.Annotated[bool, _REBUILD_LOCKFILES_PARAMETER] + +_LIVE_PARAMETER = Parameter( + name="live", + help="Run cargo publish without --dry-run; default behaviour is dry-run.", +) +LiveFlag = typ.Annotated[bool, _LIVE_PARAMETER] + +_FORBID_DIRTY_PARAMETER = Parameter( + name="forbid-dirty", + help=("Require a clean working tree before running publish pre-flight checks."), +) +ForbidDirtyFlag = typ.Annotated[bool, _FORBID_DIRTY_PARAMETER] + +_ALLOW_UNPUBLISHED_WORKSPACE_DEPS_PARAMETER = Parameter( + name="allow-unpublished-workspace-deps", + help=( + "Dry-run only: downgrade cargo package failures caused by a sibling " + "workspace crate version not yet on crates.io to a warning when the " + "missing crate is part of the planned publish set and appears earlier " + "in publish order. Defaults to enabled in dry-run mode. Cannot be " + "combined with --live." + ), +) +AllowUnpublishedWorkspaceDepsFlag = typ.Annotated[ + bool | None, _ALLOW_UNPUBLISHED_WORKSPACE_DEPS_PARAMETER +] + + +__all__ = [ + "WORKSPACE_ROOT_ENV_VAR", + "WORKSPACE_ROOT_REQUIRED_MESSAGE", + "AllowUnpublishedWorkspaceDepsFlag", + "DryRunFlag", + "ForbidDirtyFlag", + "LiveFlag", + "RebuildLockfilesFlag", + "VersionArgument", + "WorkspaceRootOption", +] diff --git a/lading/commands/bump.py b/lading/commands/bump.py index 13c3a456..95800a95 100644 --- a/lading/commands/bump.py +++ b/lading/commands/bump.py @@ -37,33 +37,33 @@ import typing as typ from lading import config as config_module -from lading.commands import bump_docs, bump_lockfiles, bump_readme, bump_toml +from lading.commands import ( + bump_docs, + bump_lockfiles, + bump_manifests, + bump_readme, + bump_toml, +) +from lading.commands.bump_manifests import ( + _WORKSPACE_SELECTORS, + _dependency_sections_for_crate, + _determine_package_selectors, + _freeze_dependency_sections, + _should_skip_crate_update, + _update_manifest, + _workspace_dependency_sections, +) from lading.commands.bump_output import BumpChanges, _format_result_message from lading.utils import normalise_workspace_root if typ.TYPE_CHECKING: from pathlib import Path + from lading.commands.bump_manifests import _BumpContext from lading.config import LadingConfig from lading.runtime import CommandRunner from lading.workspace import WorkspaceCrate, WorkspaceGraph -_WORKSPACE_SELECTORS: typ.Final[tuple[tuple[str, ...], ...]] = ( - ("package",), - ("workspace", "package"), -) - -# Derived from the canonical section vocabulary so the kind mapping cannot -# drift from ``bump_toml.DEPENDENCY_SECTIONS``. -_NORMAL_SECTION, _DEV_SECTION, _BUILD_SECTION = bump_toml.DEPENDENCY_SECTIONS - -_DEPENDENCY_SECTION_BY_KIND: typ.Final[dict[str | None, str]] = { - None: _NORMAL_SECTION, - "normal": _NORMAL_SECTION, - "dev": _DEV_SECTION, - "build": _BUILD_SECTION, -} - LOGGER = logging.getLogger(__name__) _log = LOGGER @@ -113,19 +113,6 @@ class BumpOptions: include_workspace_sections: bool = False -@dc.dataclass(frozen=True, slots=True) -class _BumpContext: - """Initialisation context for bump operations.""" - - root_path: Path - configuration: LadingConfig - workspace: WorkspaceGraph - base_options: BumpOptions - workspace_manifest: Path - excluded: frozenset[str] - updated_crate_names: frozenset[str] - - class _CrateManifestOutcome(enum.Enum): """Closed outcome of processing a single crate manifest. @@ -214,7 +201,7 @@ def _initialize_bump_context( crate.name for crate in workspace.crates if crate.name not in excluded ) workspace_manifest = root_path / "Cargo.toml" - return _BumpContext( + return bump_manifests._BumpContext( root_path=root_path, configuration=configuration, workspace=workspace, @@ -431,119 +418,7 @@ def _apply_crate_manifest_update( ) -def _determine_package_selectors( - crate_name: str, - excluded: cabc.Set[str], -) -> tuple[tuple[str, ...], ...]: - """Return package selectors for the crate, respecting exclusion rules. - - Args: - crate_name: Name of the crate to check. - excluded: Set of excluded crate names. - - Returns - ------- - Package selectors tuple, or empty tuple if crate is excluded. - - """ - return () if crate_name in excluded else (("package",),) - - -def _should_skip_crate_update( - selectors: tuple[tuple[str, ...], ...], - dependency_sections: cabc.Mapping[str, cabc.Collection[str]], -) -> bool: - """Check if a crate update should be skipped due to no work required. - - Returns - ------- - True if both selectors and dependency_sections are empty. - - """ - return not selectors and not dependency_sections - - -def _freeze_dependency_sections( - sections: cabc.Mapping[str, cabc.Collection[str]], -) -> cabc.Mapping[str, cabc.Collection[str]]: - """Return an immutable mapping for dependency sections.""" - if not sections: - return types.MappingProxyType({}) - frozen_sections = {key: tuple(sorted(names)) for key, names in sections.items()} - return types.MappingProxyType(frozen_sections) - - -def _update_manifest( - manifest_path: Path, - selectors: tuple[tuple[str, ...], ...], - target_version: str, - options: BumpOptions, -) -> bool: - """Apply ``target_version`` to each table described by ``selectors``. - - Args: - manifest_path: Path to the Cargo.toml manifest file. - selectors: Tuple of key tuples identifying version tables to update. - target_version: The target version to apply. - options: Bump options controlling dry-run, dependency sections, and - whether to include workspace-level dependency sections. - - Returns - ------- - True if any changes were made. - - """ - document = bump_toml.parse_manifest(manifest_path) - changed = False - for selector in selectors: - table = bump_toml.select_table(document, selector) - changed |= bump_toml.assign_version(table, target_version) - if options.dependency_sections: - changed |= bump_toml.update_dependency_sections( - document, - options.dependency_sections, - target_version, - include_workspace_sections=options.include_workspace_sections, - ) - if changed and not options.dry_run: - bump_toml.write_atomic_text(manifest_path, document.as_string()) - return changed - - -def _workspace_dependency_sections( - updated_crates: cabc.Set[str], -) -> dict[str, set[str]]: - """Return dependency names to update for the workspace manifest.""" - crate_names = {name for name in updated_crates if name} - if not crate_names: - return {} - return {section: set(crate_names) for section in bump_toml.DEPENDENCY_SECTIONS} - - -def _dependency_sections_for_crate( - crate: WorkspaceCrate, - updated_crates: cabc.Set[str], -) -> dict[str, set[str]]: - """Return dependency names grouped by section for ``crate``.""" - if not crate.dependencies: - return {} - if not updated_crates: - return {} - sections: dict[str, set[str]] = {} - for dependency in crate.dependencies: - if dependency.name not in updated_crates: - continue - section = _DEPENDENCY_SECTION_BY_KIND.get(dependency.kind, _NORMAL_SECTION) - # ``manifest_name`` preserves the dependency key used in the manifest. - # When a crate is aliased (e.g. ``alpha-core = { package = "alpha" }``) - # the workspace dependency name remains ``alpha`` while the manifest - # entry becomes ``alpha-core``. Recording the manifest key ensures the - # corresponding table entry can be located and updated. - sections.setdefault(section, set()).add(dependency.manifest_name) - return sections - - -# Re-export internal functions used by tests to maintain backward compatibility +# Re-export low-level TOML helpers used by tests for backward compatibility. _parse_manifest = bump_toml.parse_manifest _select_table = bump_toml.select_table _assign_version = bump_toml.assign_version diff --git a/lading/commands/bump_docs.py b/lading/commands/bump_docs.py index 9a8b4443..87631694 100644 --- a/lading/commands/bump_docs.py +++ b/lading/commands/bump_docs.py @@ -3,18 +3,9 @@ This module provides utilities for updating version references within Markdown documentation files. It scans fenced TOML code blocks (e.g., Cargo.toml snippets in README files) and rewrites version entries to match the target -workspace version. - -Functions ---------- -resolve_documentation_targets - Locate documentation files matching configured glob patterns. -update_documentation_files - Rewrite TOML fences in documentation to reflect updated versions. -rewrite_markdown_toml_fences - Parse Markdown and transform TOML code blocks. -update_toml_snippet_versions - Update version entries within a single TOML snippet. +workspace version. The public surface is ``resolve_documentation_targets``, +``update_documentation_files``, ``rewrite_markdown_toml_fences``, and +``update_toml_snippet_versions``. Examples -------- diff --git a/lading/commands/bump_manifests.py b/lading/commands/bump_manifests.py new file mode 100644 index 00000000..398d3cc3 --- /dev/null +++ b/lading/commands/bump_manifests.py @@ -0,0 +1,197 @@ +"""Manifest rewriting helpers for ``lading bump``. + +Extracted from :mod:`lading.commands.bump` (issue #108). This module owns +per-manifest version and dependency-section rewriting; orchestration and +context derivation stay in ``bump``, which re-exports these helpers for the +historical ``bump._update_manifest``-style access used by tests. +""" + +from __future__ import annotations + +import collections.abc as cabc +import dataclasses as dc +import types +import typing as typ + +from lading.commands import bump_toml + +if typ.TYPE_CHECKING: + from pathlib import Path + + from lading.commands.bump import BumpOptions + from lading.config import LadingConfig + from lading.workspace import WorkspaceCrate, WorkspaceGraph + +_WORKSPACE_SELECTORS: typ.Final[tuple[tuple[str, ...], ...]] = ( + ("package",), + ("workspace", "package"), +) + +# Derived from the canonical section vocabulary so the kind mapping cannot +# drift from ``bump_toml.DEPENDENCY_SECTIONS`` (issue #103). +_NORMAL_SECTION, _DEV_SECTION, _BUILD_SECTION = bump_toml.DEPENDENCY_SECTIONS + +_DEPENDENCY_SECTION_BY_KIND: typ.Final[dict[str | None, str]] = { + None: _NORMAL_SECTION, + "normal": _NORMAL_SECTION, + "dev": _DEV_SECTION, + "build": _BUILD_SECTION, +} + + +@dc.dataclass(frozen=True, slots=True) +class _BumpContext: + """Initialisation context for bump operations. + + Derived once by ``bump._initialize_bump_context`` and consumed by + :func:`_update_crate_manifest`; the manifest-mutation contract lives with + this extracted module rather than reaching back into ``bump`` internals. + """ + + root_path: Path + configuration: LadingConfig + workspace: WorkspaceGraph + base_options: BumpOptions + workspace_manifest: Path + excluded: frozenset[str] + updated_crate_names: frozenset[str] + + +def _determine_package_selectors( + crate_name: str, + excluded: cabc.Collection[str], +) -> tuple[tuple[str, ...], ...]: + """Return package selectors for the crate, respecting exclusion rules. + + Parameters + ---------- + crate_name + Name of the crate to check. + excluded + Collection of excluded crate names. + + Returns + ------- + tuple[tuple[str, ...], ...] + Package selectors, or an empty tuple when the crate is excluded. + + """ + return () if crate_name in excluded else (("package",),) + + +def _should_skip_crate_update( + selectors: tuple[tuple[str, ...], ...], + dependency_sections: cabc.Mapping[str, cabc.Collection[str]], +) -> bool: + """Return whether a crate update can be skipped for lack of work. + + Parameters + ---------- + selectors + Package selectors identified for the crate. + dependency_sections + Dependency sections to update for the crate. + + Returns + ------- + bool + ``True`` when both ``selectors`` and ``dependency_sections`` are empty. + + """ + return not selectors and not dependency_sections + + +def _freeze_dependency_sections( + sections: cabc.Mapping[str, cabc.Collection[str]], +) -> cabc.Mapping[str, cabc.Collection[str]]: + """Return an immutable mapping for dependency sections.""" + if not sections: + return types.MappingProxyType({}) + frozen_sections = {key: tuple(sorted(names)) for key, names in sections.items()} + return types.MappingProxyType(frozen_sections) + + +def _update_manifest( + manifest_path: Path, + selectors: tuple[tuple[str, ...], ...], + target_version: str, + options: BumpOptions, +) -> bool: + """Apply ``target_version`` to each table described by ``selectors``. + + Parameters + ---------- + manifest_path + Path to the Cargo.toml manifest file. + selectors + Key tuples identifying version tables to update. + target_version + The target version to apply. + options + Bump options controlling dry-run, dependency sections, and whether to + include workspace-level dependency sections. + + Returns + ------- + bool + ``True`` when any changes were made. + + """ + document = bump_toml.parse_manifest(manifest_path) + changed = False + for selector in selectors: + table = bump_toml.select_table(document, selector) + changed |= bump_toml.assign_version(table, target_version) + if options.dependency_sections: + changed |= bump_toml.update_dependency_sections( + document, + options.dependency_sections, + target_version, + include_workspace_sections=options.include_workspace_sections, + ) + if changed and not options.dry_run: + bump_toml.write_atomic_text(manifest_path, document.as_string()) + return changed + + +def _workspace_dependency_sections( + updated_crates: cabc.Collection[str], +) -> dict[str, set[str]]: + """Return dependency names to update for the workspace manifest.""" + crate_names = {name for name in updated_crates if name} + if not crate_names: + return {} + return {section: set(crate_names) for section in bump_toml.DEPENDENCY_SECTIONS} + + +def _dependency_sections_for_crate( + crate: WorkspaceCrate, + updated_crates: cabc.Collection[str], +) -> dict[str, set[str]]: + """Return dependency names grouped by section for ``crate``.""" + if not crate.dependencies: + return {} + targets = {name for name in updated_crates if name} + if not targets: + return {} + sections: dict[str, set[str]] = {} + for dependency in crate.dependencies: + if dependency.name not in targets: + continue + section = _DEPENDENCY_SECTION_BY_KIND.get(dependency.kind, _NORMAL_SECTION) + # ``manifest_name`` preserves the dependency key used in the manifest. + # When a crate is aliased (e.g. ``alpha-core = { package = "alpha" }``) + # the workspace dependency name remains ``alpha`` while the manifest + # entry becomes ``alpha-core``. Recording the manifest key ensures the + # corresponding table entry can be located and updated. + sections.setdefault(section, set()).add(dependency.manifest_name) + return sections + + +# Re-export internal functions used by tests to maintain backward compatibility +_parse_manifest = bump_toml.parse_manifest +_select_table = bump_toml.select_table +_assign_version = bump_toml.assign_version +_value_matches = bump_toml.value_matches +_update_dependency_sections = bump_toml.update_dependency_sections +_update_dependency_table = bump_toml.update_dependency_table diff --git a/lading/config.py b/lading/config.py index fc585fc3..bee7e59b 100644 --- a/lading/config.py +++ b/lading/config.py @@ -6,10 +6,12 @@ import contextlib import contextvars import dataclasses as dc +import functools import typing as typ from cyclopts.config import Toml +from lading import toml_coerce from lading.exceptions import LadingError from lading.utils import normalise_workspace_root @@ -301,119 +303,6 @@ def current_configuration() -> LadingConfig: raise ConfigurationNotLoadedError(message) from exc -def _validate_string_sequence( - sequence: cabc.Sequence[typ.Any], field_name: str -) -> tuple[str, ...]: - """Validate that ``sequence`` contains only strings and return them.""" - items: list[str] = [] - for index, entry in enumerate(sequence): - if not isinstance(entry, str): - message = ( - f"{field_name}[{index}] must be a string, got {type(entry).__name__}." - ) - raise ConfigurationError(message) - items.append(entry) - return tuple(items) - - -def _string_tuple(value: object, field_name: str) -> tuple[str, ...]: - """Return a tuple of strings derived from ``value``.""" - if value is None: - return () - if isinstance(value, str): - return (value,) - if isinstance(value, cabc.Sequence) and not isinstance(value, str | bytes): - return _validate_string_sequence(value, field_name) - message = ( - f"{field_name} must be a string or a sequence of strings; " - f"received {type(value).__name__}." - ) - raise ConfigurationError(message) - - -def _validate_matrix_entry( - entry: object, - field_name: str, - index: int, -) -> tuple[str, ...]: - """Validate and convert a single matrix entry to a string tuple.""" - if isinstance(entry, cabc.Sequence) and not isinstance(entry, str | bytes): - return _validate_string_sequence(entry, f"{field_name}[{index}]") - message = ( - f"{field_name}[{index}] must be a sequence of strings; " - f"received {type(entry).__name__}." - ) - raise ConfigurationError(message) - - -def _validate_string_pair( - key: object, raw_value: object, field_name: str -) -> tuple[str, str]: - """Validate and return a string key-value pair for ``field_name``.""" - if not isinstance(key, str): - message = f"{field_name} keys must be strings; received {type(key).__name__}." - raise ConfigurationError(message) - if not isinstance(raw_value, str): - message = ( - f"{field_name}[{key}] must be a string; " - f"received {type(raw_value).__name__}." - ) - raise ConfigurationError(message) - return (key, raw_value) - - -def _string_matrix(value: object, field_name: str) -> tuple[tuple[str, ...], ...]: - """Return a tuple-of-tuples parsed from ``value`` as nested string sequences.""" - if value is None: - return () - if not isinstance(value, cabc.Sequence) or isinstance(value, str | bytes): - message = f"{field_name} must be a sequence of string sequences." - raise ConfigurationError(message) - commands = [ - _validate_matrix_entry(entry, field_name, index) - for index, entry in enumerate(value) - ] - return tuple(commands) - - -def _string_mapping(value: object, field_name: str) -> tuple[tuple[str, str], ...]: - """Return key/value string pairs derived from mapping ``value``.""" - if value is None: - return () - if not isinstance(value, cabc.Mapping): - message = f"{field_name} must be a TOML table; received {type(value).__name__}." - raise ConfigurationError(message) - items: list[tuple[str, str]] = [] - for key, raw_value in value.items(): - items.append(_validate_string_pair(key, raw_value, field_name)) - return tuple(items) - - -def _non_negative_int(value: object, field_name: str, default: int) -> int: - """Return a non-negative integer parsed from ``value`` or ``default`` when None.""" - if value is None: - return default - try: - integer = int(typ.cast("typ.Any", value)) - except (TypeError, ValueError) as exc: # pragma: no cover - validation guard - message = f"{field_name} must be an integer; received {type(value).__name__}." - raise ConfigurationError(message) from exc - if integer < 0: - message = f"{field_name} must be non-negative." - raise ConfigurationError(message) - return integer - - -def _boolean(value: object, field_name: str, *, default: bool = False) -> bool: - """Return a boolean parsed from ``value``.""" - if value is None: - return default - if isinstance(value, bool): - return value - message = f"{field_name} must be a boolean; received {type(value).__name__}." - raise ConfigurationError(message) - - def _strip_patches(value: object) -> StripPatchesSetting: """Normalise the ``publish.strip_patches`` value.""" if value is None: @@ -429,13 +318,17 @@ def _strip_patches(value: object) -> StripPatchesSetting: raise ConfigurationError(message) -def _optional_mapping( - value: object, field_name: str -) -> cabc.Mapping[str, typ.Any] | None: - """Ensure ``value`` is a mapping if provided.""" - if value is None: - return None - if isinstance(value, cabc.Mapping): - return typ.cast("cabc.Mapping[str, typ.Any]", value) - message = f"{field_name} must be a TOML table; received {type(value).__name__}." - raise ConfigurationError(message) +# Coercion helpers bound to the configuration error type; the shared +# implementations live in lading.toml_coerce (issue #108). +_string_tuple = functools.partial(toml_coerce.string_tuple, error=ConfigurationError) +_string_matrix = functools.partial(toml_coerce.string_matrix, error=ConfigurationError) +_string_mapping = functools.partial( + toml_coerce.string_mapping, error=ConfigurationError +) +_boolean = functools.partial(toml_coerce.boolean, error=ConfigurationError) +_non_negative_int = functools.partial( + toml_coerce.non_negative_int, error=ConfigurationError +) +_optional_mapping = functools.partial( + toml_coerce.optional_mapping, error=ConfigurationError +) diff --git a/lading/toml_coerce/__init__.py b/lading/toml_coerce/__init__.py new file mode 100644 index 00000000..ceae95de --- /dev/null +++ b/lading/toml_coerce/__init__.py @@ -0,0 +1,29 @@ +"""Shared TOML scalar, sequence, and mapping coercion helpers.""" + +from lading.toml_coerce._mappings import ( + expect_mapping, + optional_mapping, + string_mapping, +) +from lading.toml_coerce._scalars import boolean, expect_string, non_negative_int +from lading.toml_coerce._sequences import ( + expect_sequence, + is_non_empty_sequence, + string_matrix, + string_tuple, + validate_string_sequence, +) + +__all__ = [ + "boolean", + "expect_mapping", + "expect_sequence", + "expect_string", + "is_non_empty_sequence", + "non_negative_int", + "optional_mapping", + "string_mapping", + "string_matrix", + "string_tuple", + "validate_string_sequence", +] diff --git a/lading/toml_coerce/_core.py b/lading/toml_coerce/_core.py new file mode 100644 index 00000000..d3eb29e2 --- /dev/null +++ b/lading/toml_coerce/_core.py @@ -0,0 +1,28 @@ +"""Shared error-building primitive for the TOML coercion helpers. + +Holds the :class:`~lading.exceptions.LadingError` forward reference, the +``_ErrorType`` alias naming the subclass to raise, and the :func:`_reject` +helper that produces the canonical coercion message shape: + +``{field} must be {expected}; received {type(value).__name__}.`` +""" + +from __future__ import annotations + +import typing as typ + +if typ.TYPE_CHECKING: + from lading.exceptions import LadingError + +type _ErrorType = type[LadingError] + + +def _reject( + value: object, field_name: str, expected: str, error: _ErrorType +) -> LadingError: + """Build ``error`` with the canonical coercion message shape.""" + # Callers raise the returned exception so each coercion helper terminates + # explicitly on the failure path; keeping this primitive in its own module + # preserves legibility for linters and type checkers. + message = f"{field_name} must be {expected}; received {type(value).__name__}." + return error(message) diff --git a/lading/toml_coerce/_mappings.py b/lading/toml_coerce/_mappings.py new file mode 100644 index 00000000..39b851c6 --- /dev/null +++ b/lading/toml_coerce/_mappings.py @@ -0,0 +1,125 @@ +"""Mapping coercion helpers for TOML tables.""" + +from __future__ import annotations + +import collections.abc as cabc +import typing as typ + +from lading.toml_coerce._core import _ErrorType, _reject + + +def expect_mapping( + value: object, field_name: str, *, error: _ErrorType +) -> cabc.Mapping[str, typ.Any]: + """Return ``value`` as a string-keyed mapping or raise ``error``. + + Parameters + ---------- + value : object + The value to coerce. + field_name : str + Name of the field, used in error messages. + error : _ErrorType + Exception factory called when ``value`` is not a mapping. + + Returns + ------- + collections.abc.Mapping[str, typing.Any] + ``value`` unchanged when it is a mapping. + + Raises + ------ + LadingError + When ``value`` is not a :class:`collections.abc.Mapping`. + """ + match value: + case cabc.Mapping(): + return typ.cast("cabc.Mapping[str, typ.Any]", value) + case _: + raise _reject(value, field_name, "a mapping", error) + + +def optional_mapping( + value: object, field_name: str, *, error: _ErrorType +) -> cabc.Mapping[str, typ.Any] | None: + """Return ``value`` as a mapping when provided, or ``None``. + + Parameters + ---------- + value : object + The value to coerce. ``None`` passes through unchanged. + field_name : str + Name of the field, used in error messages. + error : _ErrorType + Exception factory called when ``value`` is neither ``None`` nor a + mapping. + + Returns + ------- + collections.abc.Mapping[str, typing.Any] | None + The mapping, or ``None`` when ``value`` is ``None``. + + Raises + ------ + LadingError + When ``value`` is neither ``None`` nor a mapping. + """ + if value is None: + return None + return expect_mapping(value, field_name, error=error) + + +def _validate_string_pair( + key: object, raw_value: object, field_name: str, error: _ErrorType +) -> tuple[str, str]: + """Validate and return a string key-value pair for ``field_name``.""" + match key: + case str(): + match raw_value: + case str(): + return (key, raw_value) + case _: + raise _reject(raw_value, f"{field_name}[{key}]", "a string", error) + case _: + message = ( + f"{field_name} keys must be strings; received {type(key).__name__}." + ) + raise error(message) + + +def string_mapping( + value: object, field_name: str, *, error: _ErrorType +) -> tuple[tuple[str, str], ...]: + """Return key/value string pairs derived from mapping ``value``. + + Parameters + ---------- + value : object + The value to coerce. ``None`` yields an empty tuple. + field_name : str + Name of the field, used in error messages. + error : _ErrorType + Exception factory called when validation fails. + + Returns + ------- + tuple[tuple[str, str], ...] + The validated ``(key, value)`` string pairs; empty when ``value`` is + ``None``. + + Raises + ------ + LadingError + When ``value`` is not a TOML table, or when any key or value is not a + string. + """ + match value: + case None: + return () + case cabc.Mapping(): + return tuple( + _validate_string_pair(key, raw_value, field_name, error) + for key, raw_value in value.items() + ) + case _: + raise _reject(value, field_name, "a TOML table", error) diff --git a/lading/toml_coerce/_scalars.py b/lading/toml_coerce/_scalars.py new file mode 100644 index 00000000..2599612b --- /dev/null +++ b/lading/toml_coerce/_scalars.py @@ -0,0 +1,120 @@ +"""Scalar coercion helpers for TOML values (strings, booleans, integers).""" + +from __future__ import annotations + +from lading.toml_coerce._core import _ErrorType, _reject + + +def expect_string(value: object, field_name: str, *, error: _ErrorType) -> str: + """Return ``value`` when it is a string, otherwise raise ``error``. + + Parameters + ---------- + value : object + The value to coerce. + field_name : str + Name of the field, used in error messages. + error : _ErrorType + Exception factory called when ``value`` is not a string. + + Returns + ------- + str + ``value`` unchanged. + + Raises + ------ + LadingError + When ``value`` is not a :class:`str`. + """ + match value: + case str(): + return value + case _: + raise _reject(value, field_name, "a string", error) + + +def boolean( + value: object, field_name: str, *, error: _ErrorType, default: bool = False +) -> bool: + """Return a boolean parsed from ``value`` or ``default`` when ``None``. + + Parameters + ---------- + value : object + The value to coerce. + field_name : str + Name of the field, used in error messages. + error : _ErrorType + Exception factory called when ``value`` is neither ``None`` nor a bool. + default : bool, optional + Value returned when ``value`` is ``None`` (default ``False``). + + Returns + ------- + bool + The boolean, or ``default`` when ``value`` is ``None``. + + Raises + ------ + LadingError + When ``value`` is neither ``None`` nor a :class:`bool`. + """ + match value: + case None: + return default + case bool(): + return value + case _: + raise _reject(value, field_name, "a boolean", error) + + +def non_negative_int( + value: object, field_name: str, default: int, *, error: _ErrorType +) -> int: + """Return a non-negative integer parsed from ``value`` or ``default``. + + Parameters + ---------- + value : object + The value to coerce. ``None`` selects ``default``. + field_name : str + Name of the field, used in error messages. + default : int + Value returned when ``value`` is ``None``. + error : _ErrorType + Exception factory called when validation fails. + + Returns + ------- + int + A non-negative integer parsed from ``value``, or ``default``. + + Raises + ------ + LadingError + When ``value`` is not a real integer (``bool`` and ``float`` are + rejected) or an integer-valued string, or when the result is negative. + """ + type_error = f"{field_name} must be an integer; received {type(value).__name__}." + # ``bool`` is a subclass of ``int`` and ``float``/other types are truthy for + # a blanket ``int(...)`` cast, so dispatch explicitly to accept only real + # integers and integer-valued strings (the config string path). + match value: + case None: + return default + case bool(): + raise error(type_error) + case int(): + integer = value + case str(): + try: + integer = int(value) + except ValueError as exc: + raise error(type_error) from exc + case _: + raise error(type_error) + if integer < 0: + message = f"{field_name} must be non-negative." + raise error(message) + return integer diff --git a/lading/toml_coerce/_sequences.py b/lading/toml_coerce/_sequences.py new file mode 100644 index 00000000..f282e1e1 --- /dev/null +++ b/lading/toml_coerce/_sequences.py @@ -0,0 +1,140 @@ +"""Sequence coercion helpers for TOML values (tuples and matrices).""" + +from __future__ import annotations + +import collections.abc as cabc +import typing as typ + +from lading.toml_coerce._core import _ErrorType, _reject + + +@typ.overload +def expect_sequence( + value: object, + field_name: str, + *, + error: _ErrorType, + allow_none: typ.Literal[False] = False, +) -> cabc.Sequence[object]: + """Require a sequence when ``allow_none`` is ``False``.""" + ... # pylint: disable=unnecessary-ellipsis + + +@typ.overload +def expect_sequence( + value: object, + field_name: str, + *, + error: _ErrorType, + allow_none: typ.Literal[True], +) -> cabc.Sequence[object] | None: + """Allow ``None`` when ``allow_none`` is ``True``.""" + ... # pylint: disable=unnecessary-ellipsis + + +def expect_sequence( + value: object, + field_name: str, + *, + error: _ErrorType, + allow_none: bool = False, +) -> cabc.Sequence[object] | None: + """Ensure ``value`` is a non-string sequence (optionally ``None``).""" + match value: + case None: + if allow_none: + return None + message = f"{field_name} must be a sequence" + raise error(message) + case str() | bytes() | bytearray(): + raise _reject(value, field_name, "a sequence", error) + case cabc.Sequence(): + return value + case _: + raise _reject(value, field_name, "a sequence", error) + + +def is_non_empty_sequence(value: object) -> bool: + """Return ``True`` when ``value`` is a non-string sequence with content.""" + match value: + case str() | bytes() | bytearray(): + return False + case cabc.Sequence(): + return bool(value) + case _: + return False + + +def validate_string_sequence( + sequence: cabc.Sequence[typ.Any], field_name: str, *, error: _ErrorType +) -> tuple[str, ...]: + """Validate that ``sequence`` contains only strings and return them.""" + items: list[str] = [] + for index, entry in enumerate(sequence): + match entry: + case str(): + items.append(entry) + case _: + raise _reject(entry, f"{field_name}[{index}]", "a string", error) + return tuple(items) + + +def string_tuple( + value: object, field_name: str, *, error: _ErrorType +) -> tuple[str, ...]: + """Return a tuple of strings derived from ``value``.""" + # ``bytearray`` is deliberately excluded from the string/bytes rejection so + # it flows into ``validate_string_sequence`` (which rejects its int items), + # preserving the pre-refactor behaviour. + match value: + case None: + return () + case str(): + return (value,) + case bytes(): + raise _reject(value, field_name, "a string or a sequence of strings", error) + case cabc.Sequence(): + return validate_string_sequence(value, field_name, error=error) + case _: + raise _reject(value, field_name, "a string or a sequence of strings", error) + + +def _validate_matrix_entry( + entry: object, + field_name: str, + index: int, + error: _ErrorType, +) -> tuple[str, ...]: + """Validate and convert a single matrix entry to a string tuple.""" + match entry: + case str() | bytes(): + raise _reject( + entry, f"{field_name}[{index}]", "a sequence of strings", error + ) + case cabc.Sequence(): + return validate_string_sequence( + entry, f"{field_name}[{index}]", error=error + ) + case _: + raise _reject( + entry, f"{field_name}[{index}]", "a sequence of strings", error + ) + + +def string_matrix( + value: object, field_name: str, *, error: _ErrorType +) -> tuple[tuple[str, ...], ...]: + """Return a tuple-of-tuples parsed from ``value`` as string sequences.""" + message = f"{field_name} must be a sequence of string sequences." + match value: + case None: + return () + case str() | bytes(): + raise error(message) + case cabc.Sequence(): + return tuple( + _validate_matrix_entry(entry, field_name, index, error) + for index, entry in enumerate(value) + ) + case _: + raise error(message) diff --git a/lading/workspace/__init__.py b/lading/workspace/__init__.py index 0f086ac7..b9db2b61 100644 --- a/lading/workspace/__init__.py +++ b/lading/workspace/__init__.py @@ -2,6 +2,10 @@ from __future__ import annotations +from .graph_build import ( + build_workspace_graph, + load_workspace, +) from .metadata import ( CargoExecutableNotFoundError, CargoMetadataError, @@ -13,8 +17,6 @@ WorkspaceDependencyCycleError, WorkspaceGraph, WorkspaceModelError, - build_workspace_graph, - load_workspace, ) __all__ = [ diff --git a/lading/workspace/_coercion.py b/lading/workspace/_coercion.py new file mode 100644 index 00000000..05fd912d --- /dev/null +++ b/lading/workspace/_coercion.py @@ -0,0 +1,65 @@ +"""Workspace coercion helpers bound to :class:`WorkspaceModelError`. + +The shared TOML coercion primitives live in :mod:`lading.toml_coerce`; this +module binds them to the workspace error type so the workspace builders in +:mod:`lading.workspace.graph_build` do not reach into :mod:`lading.workspace.models` +internals for them (issue #108). ``models`` defines the model types only; +``_coercion`` owns the error-bound coercion contract shared with the builders. +""" + +from __future__ import annotations + +import collections.abc as cabc +import functools +import typing as typ + +from lading import toml_coerce +from lading.workspace.models import WorkspaceModelError + +_expect_mapping = functools.partial( + toml_coerce.expect_mapping, error=WorkspaceModelError +) + + +@typ.overload +def _expect_sequence( + value: object, + field_name: str, + *, + allow_none: typ.Literal[False] = False, +) -> cabc.Sequence[object]: + """Require a sequence when ``allow_none`` is ``False``.""" + ... # pylint: disable=unnecessary-ellipsis + + +@typ.overload +def _expect_sequence( + value: object, + field_name: str, + *, + allow_none: typ.Literal[True], +) -> cabc.Sequence[object] | None: + """Allow ``None`` when ``allow_none`` is ``True``.""" + ... # pylint: disable=unnecessary-ellipsis + + +def _expect_sequence( + value: object, + field_name: str, + *, + allow_none: bool = False, +) -> cabc.Sequence[object] | None: + """Bind :func:`toml_coerce.expect_sequence` to ``WorkspaceModelError``. + + A typed wrapper (rather than ``functools.partial``) preserves the + overloads that narrow the return type when ``allow_none`` is false. + """ + if allow_none: + return toml_coerce.expect_sequence( + value, field_name, error=WorkspaceModelError, allow_none=True + ) + return toml_coerce.expect_sequence(value, field_name, error=WorkspaceModelError) + + +_expect_string = functools.partial(toml_coerce.expect_string, error=WorkspaceModelError) +_is_non_empty_sequence = toml_coerce.is_non_empty_sequence diff --git a/lading/workspace/graph_build.py b/lading/workspace/graph_build.py new file mode 100644 index 00000000..b7edc265 --- /dev/null +++ b/lading/workspace/graph_build.py @@ -0,0 +1,398 @@ +"""Builders converting ``cargo metadata`` output into workspace models. + +Extracted from :mod:`lading.workspace.models` (issue #108) so the data +structures and topology stay separate from the metadata-parsing layer. The +module shares the coercion bindings defined in ``models`` so all workspace +validation failures raise :class:`WorkspaceModelError` with the canonical +message shape from :mod:`lading.toml_coerce`. +""" + +from __future__ import annotations + +import collections.abc as cabc +import typing as typ +from pathlib import Path + +from tomlkit import parse +from tomlkit.exceptions import TOMLKitError + +from lading.workspace._coercion import ( + _expect_mapping, + _expect_sequence, + _expect_string, + _is_non_empty_sequence, +) +from lading.workspace.models import ( + ALLOWED_DEP_KINDS, + WORKSPACE_ROOT_MISSING_MSG, + WorkspaceCrate, + WorkspaceDependency, + WorkspaceGraph, + WorkspaceIndex, + WorkspaceModelError, +) + + +def load_workspace( + workspace_root: Path | str | None = None, +) -> WorkspaceGraph: + """Return a :class:`WorkspaceGraph` constructed from ``cargo metadata``. + + Parameters + ---------- + workspace_root : Path | str | None + Directory to run ``cargo metadata`` in; ``None`` uses cargo's default. + + Returns + ------- + WorkspaceGraph + The workspace graph built from the discovered cargo metadata. + + Raises + ------ + CargoExecutableNotFoundError, CargoMetadataError + When ``cargo`` cannot be located, or ``cargo metadata`` fails or emits + unparsable output. + WorkspaceModelError + When the metadata cannot be turned into a valid workspace graph. + """ + from lading.workspace.metadata import load_cargo_metadata + + metadata = load_cargo_metadata(workspace_root) + return build_workspace_graph(metadata) + + +def build_workspace_graph( + metadata: cabc.Mapping[str, typ.Any], +) -> WorkspaceGraph: + """Convert ``cargo metadata`` output into :class:`WorkspaceGraph`. + + Parameters + ---------- + metadata : collections.abc.Mapping[str, typing.Any] + Parsed ``cargo metadata`` with ``workspace_root`` (path string), + ``packages`` (package mappings), and ``workspace_members`` (id + strings). + + Returns + ------- + WorkspaceGraph + The crates and dependency relationships derived from ``metadata``. + + Raises + ------ + WorkspaceModelError + When ``workspace_root`` is missing, the ``packages``/ + ``workspace_members`` sequences are malformed, or a member is unknown. + """ + try: + workspace_root_value = metadata["workspace_root"] + except KeyError as exc: + raise WorkspaceModelError(WORKSPACE_ROOT_MISSING_MSG) from exc + workspace_root = _normalise_workspace_root(workspace_root_value) + packages = _expect_sequence(metadata.get("packages"), "packages", allow_none=False) + workspace_members = _expect_sequence( + metadata.get("workspace_members"), "workspace_members", allow_none=False + ) + workspace_member_ids = tuple( + _expect_string(member, "workspace_members[]") for member in workspace_members + ) + package_lookup = _index_workspace_packages(packages, workspace_member_ids) + workspace_index = _build_workspace_index(package_lookup) + crates = _collect_workspace_crates( + package_lookup=package_lookup, + workspace_member_ids=workspace_member_ids, + workspace_index=workspace_index, + ) + return WorkspaceGraph(workspace_root=workspace_root, crates=crates) + + +def _collect_workspace_crates( + package_lookup: dict[str, cabc.Mapping[str, typ.Any]], + workspace_member_ids: cabc.Sequence[str], + workspace_index: WorkspaceIndex, +) -> tuple[WorkspaceCrate, ...]: + """Return a :class:`WorkspaceCrate` tuple for each workspace member ID.""" + crates: list[WorkspaceCrate] = [] + for member_id in workspace_member_ids: + raw_package = package_lookup.get(member_id) + if raw_package is None: + message = f"workspace member {member_id!r} missing from package list" + raise WorkspaceModelError(message) + crates.append(_build_crate(raw_package, workspace_index)) + return tuple(crates) + + +def _index_workspace_packages( + packages: cabc.Sequence[object], + workspace_member_ids: cabc.Sequence[str], +) -> dict[str, cabc.Mapping[str, typ.Any]]: + """Return mapping of workspace member IDs to package metadata.""" + member_set = set(workspace_member_ids) + index: dict[str, cabc.Mapping[str, typ.Any]] = {} + for package in packages: + package_mapping = _expect_mapping(package, "packages[]") + package_id = _expect_string(package_mapping.get("id"), "packages[].id") + if package_id not in member_set: + continue + index[package_id] = package_mapping + return index + + +def _build_crate( + package: cabc.Mapping[str, typ.Any], + workspace_index: WorkspaceIndex, +) -> WorkspaceCrate: + """Construct a :class:`WorkspaceCrate` from ``cargo metadata`` package data.""" + package_id = _expect_string(package.get("id"), "packages[].id") + name = _expect_string(package.get("name"), f"package {package_id!r} name") + version = _expect_string(package.get("version"), f"package {package_id!r} version") + manifest_path = _normalise_manifest_path( + package.get("manifest_path"), f"package {package_id!r} manifest_path" + ) + dependencies = _build_dependencies(package, workspace_index) + publish = _coerce_publish_setting(package.get("publish"), package_id) + readme_is_workspace = _manifest_uses_workspace_readme(manifest_path) + root_path = manifest_path.parent + return WorkspaceCrate( + id=package_id, + name=name, + version=version, + manifest_path=manifest_path, + root_path=root_path, + publish=publish, + readme_is_workspace=readme_is_workspace, + dependencies=dependencies, + ) + + +def _build_dependencies( + package: cabc.Mapping[str, typ.Any], + workspace_index: WorkspaceIndex, +) -> tuple[WorkspaceDependency, ...]: + """Return dependencies that reference other workspace members.""" + raw_dependencies = _expect_sequence( + package.get("dependencies"), + f"package {package.get('id')!r} dependencies", + allow_none=True, + ) + if raw_dependencies is None: + return () + return tuple( + dependency + for dependency in ( + _as_workspace_dependency(entry, workspace_index) + for entry in raw_dependencies + ) + if dependency is not None + ) + + +def _build_workspace_index( + package_lookup: cabc.Mapping[str, cabc.Mapping[str, typ.Any]], +) -> WorkspaceIndex: + """Return workspace package lookups keyed by id and package name.""" + members_by_name: dict[str, str] = {} + for package_id, package in package_lookup.items(): + package_name = _expect_string( + package.get("name"), f"package {package_id!r} name" + ) + existing_id = members_by_name.get(package_name) + if existing_id is not None and existing_id != package_id: + message = ( + f"workspace package name {package_name!r} maps to multiple ids: " + f"{existing_id!r}, {package_id!r}" + ) + raise WorkspaceModelError(message) + members_by_name[package_name] = package_id + return WorkspaceIndex(packages=package_lookup, members_by_name=members_by_name) + + +def _validate_dependency_mapping( + entry: cabc.Mapping[str, typ.Any] | object, +) -> cabc.Mapping[str, typ.Any]: + """Return ``entry`` as a mapping or raise if it is not.""" + if not isinstance(entry, cabc.Mapping): + message = "dependency entries must be mappings" + raise WorkspaceModelError(message) + return typ.cast("cabc.Mapping[str, typ.Any]", entry) + + +def _validate_workspace_dependency_path( + entry: cabc.Mapping[str, typ.Any], + target_package: cabc.Mapping[str, typ.Any], +) -> bool: + """Return whether an entry path matches the workspace dependency target.""" + dependency_path = entry.get("path") + if dependency_path is None: + return True + if not isinstance(dependency_path, str): + return False + target_manifest_path = _normalise_manifest_path( + target_package.get("manifest_path"), + "dependency target manifest_path", + ) + dependency_root = Path(dependency_path).expanduser().resolve(strict=False) + return dependency_root == target_manifest_path.parent + + +def _lookup_workspace_target( + entry: cabc.Mapping[str, typ.Any], + workspace_index: WorkspaceIndex, +) -> tuple[str, str] | None: + """Return the dependency target id and name when in the workspace.""" + # External sources should never resolve to workspace dependencies. + if entry.get("source") is not None: + return None + for candidate_name in _dependency_candidate_names(entry): + target_id = workspace_index.members_by_name.get(candidate_name) + if target_id is None: + continue + target_package = workspace_index.packages.get(target_id) + if target_package is None: + continue + if not _validate_workspace_dependency_path(entry, target_package): + continue + + target_name = _expect_string( + target_package.get("name"), f"package {target_id!r} name" + ) + return target_id, target_name + return None + + +def _dependency_candidate_names(entry: cabc.Mapping[str, typ.Any]) -> tuple[str, ...]: + """Return candidate dependency package names from metadata.""" + names: list[str] = [] + dependency_name = entry.get("name") + if isinstance(dependency_name, str): + names.append(dependency_name) + # Some metadata producers include `package` for canonical dependency names. + package_name = entry.get("package") + if isinstance(package_name, str) and package_name not in names: + names.append(package_name) + return tuple(names) + + +def _validate_dependency_kind( + entry: cabc.Mapping[str, typ.Any], +) -> typ.Literal["normal", "dev", "build"] | None: + """Return a validated dependency kind literal when present.""" + kind_value = entry.get("kind") + if kind_value is None: + return None + if not isinstance(kind_value, str): + message = ( + f"dependency kind must be string; received {type(kind_value).__name__}" + ) + raise WorkspaceModelError(message) + if kind_value not in ALLOWED_DEP_KINDS: + message = f"unsupported dependency kind {kind_value!r}" + raise WorkspaceModelError(message) + return typ.cast("typ.Literal['normal', 'dev', 'build']", kind_value) + + +def _as_workspace_dependency( + entry: cabc.Mapping[str, typ.Any] | object, + workspace_index: WorkspaceIndex, +) -> WorkspaceDependency | None: + """Convert ``entry`` into a :class:`WorkspaceDependency` when possible.""" + dependency = _validate_dependency_mapping(entry) + target = _lookup_workspace_target(dependency, workspace_index) + if target is None: + return None + target_id, target_name = target + manifest_name = _dependency_manifest_name(dependency, target_id) + kind_literal = _validate_dependency_kind(dependency) + return WorkspaceDependency( + package_id=target_id, + name=target_name, + manifest_name=manifest_name, + kind=kind_literal, + ) + + +def _dependency_manifest_name( + dependency: cabc.Mapping[str, typ.Any], + target_id: str, +) -> str: + """Return the dependency name used in manifests.""" + rename_value = dependency.get("rename") + if isinstance(rename_value, str) and rename_value: + return rename_value + dependency_name = dependency.get("name") + if isinstance(dependency_name, str): + return dependency_name + package_name = dependency.get("package") + if isinstance(package_name, str): + return package_name + return _expect_string( + dependency_name, + f"dependency {target_id!r} name", + ) + + +def _normalise_workspace_root(value: object) -> Path: + """Return ``value`` as an absolute workspace root path.""" + if not isinstance(value, str | Path): + message = ( + f"workspace_root must be a path string; received {type(value).__name__}" + ) + raise WorkspaceModelError(message) + from lading.utils.path import normalise_workspace_root + + return normalise_workspace_root(value) + + +def _normalise_manifest_path(value: object, field_name: str) -> Path: + """Return ``value`` as an absolute :class:`Path` to a manifest.""" + if not isinstance(value, str | Path): + message = f"{field_name} must be a path string; received {type(value).__name__}" + raise WorkspaceModelError(message) + path_value = Path(value).expanduser() + return path_value.resolve(strict=False) + + +def _coerce_publish_setting(value: object, package_id: str) -> bool: + """Return whether ``package_id`` should be considered publishable.""" + if value is None: + return True + if isinstance(value, bool): + return value + if isinstance(value, cabc.Sequence) and not isinstance( + value, str | bytes | bytearray + ): + return _is_non_empty_sequence(value) + message = ( + f"publish setting for package {package_id!r} must be false, a list, or null" + ) + raise WorkspaceModelError(message) + + +def _extract_readme_workspace_flag(package_table: object) -> bool: + """Return ``True`` when ``package_table`` opts into workspace readme.""" + if not isinstance(package_table, cabc.Mapping): + return False + package_mapping = typ.cast("cabc.Mapping[str, typ.Any]", package_table) + readme_value = package_mapping.get("readme") + if not isinstance(readme_value, cabc.Mapping): + return False + readme_mapping = typ.cast("cabc.Mapping[str, typ.Any]", readme_value) + workspace_flag = readme_mapping.get("workspace") + return bool(workspace_flag) + + +def _manifest_uses_workspace_readme(manifest_path: Path) -> bool: + """Return ``True`` when ``readme.workspace`` is set in ``manifest_path``.""" + try: + text = manifest_path.read_text(encoding="utf-8") + except FileNotFoundError as exc: # pragma: no cover - defensive guard + message = f"manifest not found: {manifest_path}" + raise WorkspaceModelError(message) from exc + try: + document = parse(text) + except TOMLKitError as exc: + message = f"failed to parse manifest {manifest_path}: {exc}" + raise WorkspaceModelError(message) from exc + package_table = document.get("package") + return _extract_readme_workspace_flag(package_table) diff --git a/lading/workspace/models.py b/lading/workspace/models.py index 10ef5ae9..1c2d44ec 100644 --- a/lading/workspace/models.py +++ b/lading/workspace/models.py @@ -10,8 +10,6 @@ from pathlib import Path import msgspec -from tomlkit import parse -from tomlkit.exceptions import TOMLKitError from lading.exceptions import LadingError @@ -182,395 +180,15 @@ class WorkspaceIndex: members_by_name: cabc.Mapping[str, str] -def load_workspace( - workspace_root: Path | str | None = None, -) -> WorkspaceGraph: - """Return a :class:`WorkspaceGraph` constructed from ``cargo metadata``.""" - from lading.workspace.metadata import load_cargo_metadata - - metadata = load_cargo_metadata(workspace_root) - return build_workspace_graph(metadata) - - -def build_workspace_graph( - metadata: cabc.Mapping[str, typ.Any], -) -> WorkspaceGraph: - """Convert ``cargo metadata`` output into :class:`WorkspaceGraph`.""" - try: - workspace_root_value = metadata["workspace_root"] - except KeyError as exc: - raise WorkspaceModelError(WORKSPACE_ROOT_MISSING_MSG) from exc - workspace_root = _normalise_workspace_root(workspace_root_value) - packages = _expect_sequence(metadata.get("packages"), "packages", allow_none=False) - workspace_members = _expect_sequence( - metadata.get("workspace_members"), "workspace_members", allow_none=False - ) - workspace_member_ids = tuple( - _expect_string(member, "workspace_members[]") for member in workspace_members - ) - package_lookup = _index_workspace_packages(packages, workspace_member_ids) - workspace_index = _build_workspace_index(package_lookup) - crates = _collect_workspace_crates( - package_lookup=package_lookup, - workspace_member_ids=workspace_member_ids, - workspace_index=workspace_index, - ) - return WorkspaceGraph(workspace_root=workspace_root, crates=crates) - - -def _collect_workspace_crates( - package_lookup: dict[str, cabc.Mapping[str, typ.Any]], - workspace_member_ids: cabc.Sequence[str], - workspace_index: WorkspaceIndex, -) -> tuple[WorkspaceCrate, ...]: - """Return a :class:`WorkspaceCrate` tuple for each workspace member ID.""" - crates: list[WorkspaceCrate] = [] - for member_id in workspace_member_ids: - raw_package = package_lookup.get(member_id) - if raw_package is None: - message = f"workspace member {member_id!r} missing from package list" - raise WorkspaceModelError(message) - crates.append(_build_crate(raw_package, workspace_index)) - return tuple(crates) - - -def _index_workspace_packages( - packages: cabc.Sequence[object], - workspace_member_ids: cabc.Sequence[str], -) -> dict[str, cabc.Mapping[str, typ.Any]]: - """Return mapping of workspace member IDs to package metadata.""" - member_set = set(workspace_member_ids) - index: dict[str, cabc.Mapping[str, typ.Any]] = {} - for package in packages: - package_mapping = _expect_mapping(package, "packages[]") - package_id = _expect_string(package_mapping.get("id"), "packages[].id") - if package_id not in member_set: - continue - index[package_id] = package_mapping - return index - - -def _build_crate( - package: cabc.Mapping[str, typ.Any], - workspace_index: WorkspaceIndex, -) -> WorkspaceCrate: - """Construct a :class:`WorkspaceCrate` from ``cargo metadata`` package data.""" - package_id = _expect_string(package.get("id"), "packages[].id") - name = _expect_string(package.get("name"), f"package {package_id!r} name") - version = _expect_string(package.get("version"), f"package {package_id!r} version") - manifest_path = _normalise_manifest_path( - package.get("manifest_path"), f"package {package_id!r} manifest_path" - ) - dependencies = _build_dependencies(package, workspace_index) - publish = _coerce_publish_setting(package.get("publish"), package_id) - readme_is_workspace = _manifest_uses_workspace_readme(manifest_path) - root_path = manifest_path.parent - return WorkspaceCrate( - id=package_id, - name=name, - version=version, - manifest_path=manifest_path, - root_path=root_path, - publish=publish, - readme_is_workspace=readme_is_workspace, - dependencies=dependencies, - ) - - -def _build_dependencies( - package: cabc.Mapping[str, typ.Any], - workspace_index: WorkspaceIndex, -) -> tuple[WorkspaceDependency, ...]: - """Return dependencies that reference other workspace members.""" - raw_dependencies = _expect_sequence( - package.get("dependencies"), - f"package {package.get('id')!r} dependencies", - allow_none=True, - ) - if raw_dependencies is None: - return () - return tuple( - dependency - for dependency in ( - _as_workspace_dependency(entry, workspace_index) - for entry in raw_dependencies - ) - if dependency is not None - ) - - -def _build_workspace_index( - package_lookup: cabc.Mapping[str, cabc.Mapping[str, typ.Any]], -) -> WorkspaceIndex: - """Return workspace package lookups keyed by id and package name.""" - members_by_name: dict[str, str] = {} - for package_id, package in package_lookup.items(): - package_name = _expect_string( - package.get("name"), f"package {package_id!r} name" - ) - existing_id = members_by_name.get(package_name) - if existing_id is not None and existing_id != package_id: - message = ( - f"workspace package name {package_name!r} maps to multiple ids: " - f"{existing_id!r}, {package_id!r}" - ) - raise WorkspaceModelError(message) - members_by_name[package_name] = package_id - return WorkspaceIndex(packages=package_lookup, members_by_name=members_by_name) - - -def _validate_dependency_mapping( - entry: cabc.Mapping[str, typ.Any] | object, -) -> cabc.Mapping[str, typ.Any]: - """Return ``entry`` as a mapping or raise if it is not.""" - if not isinstance(entry, cabc.Mapping): - message = "dependency entries must be mappings" - raise WorkspaceModelError(message) - return typ.cast("cabc.Mapping[str, typ.Any]", entry) - - -def _validate_workspace_dependency_path( - entry: cabc.Mapping[str, typ.Any], - target_package: cabc.Mapping[str, typ.Any], -) -> bool: - """Return whether an entry path matches the workspace dependency target.""" - dependency_path = entry.get("path") - if dependency_path is None: - return True - if not isinstance(dependency_path, str): - return False - target_manifest_path = _normalise_manifest_path( - target_package.get("manifest_path"), - "dependency target manifest_path", - ) - dependency_root = Path(dependency_path).expanduser().resolve(strict=False) - return dependency_root == target_manifest_path.parent - - -def _lookup_workspace_target( - entry: cabc.Mapping[str, typ.Any], - workspace_index: WorkspaceIndex, -) -> tuple[str, str] | None: - """Return the dependency target id and name when in the workspace.""" - # External sources should never resolve to workspace dependencies. - if entry.get("source") is not None: - return None - for candidate_name in _dependency_candidate_names(entry): - target_id = workspace_index.members_by_name.get(candidate_name) - if target_id is None: - continue - target_package = workspace_index.packages.get(target_id) - if target_package is None: - continue - if not _validate_workspace_dependency_path(entry, target_package): - continue - - target_name = _expect_string( - target_package.get("name"), f"package {target_id!r} name" - ) - return target_id, target_name - return None - - -def _dependency_candidate_names(entry: cabc.Mapping[str, typ.Any]) -> tuple[str, ...]: - """Return candidate dependency package names from metadata.""" - names: list[str] = [] - dependency_name = entry.get("name") - if isinstance(dependency_name, str): - names.append(dependency_name) - # Some metadata producers include `package` for canonical dependency names. - package_name = entry.get("package") - if isinstance(package_name, str) and package_name not in names: - names.append(package_name) - return tuple(names) - - -def _validate_dependency_kind( - entry: cabc.Mapping[str, typ.Any], -) -> typ.Literal["normal", "dev", "build"] | None: - """Return a validated dependency kind literal when present.""" - kind_value = entry.get("kind") - if kind_value is None: - return None - if not isinstance(kind_value, str): - message = ( - f"dependency kind must be string; received {type(kind_value).__name__}" - ) - raise WorkspaceModelError(message) - if kind_value not in ALLOWED_DEP_KINDS: - message = f"unsupported dependency kind {kind_value!r}" - raise WorkspaceModelError(message) - return typ.cast("typ.Literal['normal', 'dev', 'build']", kind_value) - - -def _as_workspace_dependency( - entry: cabc.Mapping[str, typ.Any] | object, - workspace_index: WorkspaceIndex, -) -> WorkspaceDependency | None: - """Convert ``entry`` into a :class:`WorkspaceDependency` when possible.""" - dependency = _validate_dependency_mapping(entry) - target = _lookup_workspace_target(dependency, workspace_index) - if target is None: - return None - target_id, target_name = target - manifest_name = _dependency_manifest_name(dependency, target_id) - kind_literal = _validate_dependency_kind(dependency) - return WorkspaceDependency( - package_id=target_id, - name=target_name, - manifest_name=manifest_name, - kind=kind_literal, - ) - - -def _dependency_manifest_name( - dependency: cabc.Mapping[str, typ.Any], - target_id: str, -) -> str: - """Return the dependency name used in manifests.""" - rename_value = dependency.get("rename") - if isinstance(rename_value, str) and rename_value: - return rename_value - dependency_name = dependency.get("name") - if isinstance(dependency_name, str): - return dependency_name - package_name = dependency.get("package") - if isinstance(package_name, str): - return package_name - return _expect_string( - dependency_name, - f"dependency {target_id!r} name", - ) - - -def _normalise_workspace_root(value: object) -> Path: - """Return ``value`` as an absolute workspace root path.""" - if not isinstance(value, str | Path): - message = ( - f"workspace_root must be a path string; received {type(value).__name__}" - ) - raise WorkspaceModelError(message) - from lading.utils.path import normalise_workspace_root - - return normalise_workspace_root(value) - - -def _normalise_manifest_path(value: object, field_name: str) -> Path: - """Return ``value`` as an absolute :class:`Path` to a manifest.""" - if not isinstance(value, str | Path): - message = f"{field_name} must be a path string; received {type(value).__name__}" - raise WorkspaceModelError(message) - path_value = Path(value).expanduser() - return path_value.resolve(strict=False) - - -def _expect_mapping(value: object, field_name: str) -> cabc.Mapping[str, typ.Any]: - """Return ``value`` as a string-keyed mapping or raise an error.""" - if isinstance(value, cabc.Mapping): - return typ.cast("cabc.Mapping[str, typ.Any]", value) - message = f"{field_name} must be a mapping; received {type(value).__name__}" - raise WorkspaceModelError(message) - - -@typ.overload -def _expect_sequence( - value: None, - field_name: str, - *, - allow_none: typ.Literal[True], -) -> None: - """Allow ``None`` only when ``allow_none`` is ``True``.""" - ... # pylint: disable=unnecessary-ellipsis - - -@typ.overload -def _expect_sequence( - value: object, - field_name: str, - *, - allow_none: typ.Literal[False] = ..., -) -> cabc.Sequence[object]: - """Require a sequence when ``allow_none`` is ``False``.""" - ... # pylint: disable=unnecessary-ellipsis - - -def _expect_sequence( - value: object, - field_name: str, - *, - allow_none: bool = False, -) -> cabc.Sequence[object] | None: - """Ensure ``value`` is a sequence (optionally ``None``).""" - if value is None: - if allow_none: - return None - message = f"{field_name} must be a sequence" - raise WorkspaceModelError(message) - if isinstance(value, cabc.Sequence) and not isinstance( - value, str | bytes | bytearray - ): - return value - message = f"{field_name} must be a sequence; received {type(value).__name__}" - raise WorkspaceModelError(message) - - -def _expect_string(value: object, field_name: str) -> str: - """Return ``value`` when it is a string, otherwise raise an error.""" - if isinstance(value, str): - return value - message = f"{field_name} must be a string; received {type(value).__name__}" - raise WorkspaceModelError(message) - - -def _is_non_empty_sequence(value: object) -> bool: - """Return ``True`` when ``value`` is a non-string sequence with content.""" - if not isinstance(value, cabc.Sequence): - return False - if isinstance(value, str | bytes | bytearray): - return False - return bool(value) - - -def _coerce_publish_setting(value: object, package_id: str) -> bool: - """Return whether ``package_id`` should be considered publishable.""" - if value is None: - return True - if isinstance(value, bool): - return value - if isinstance(value, cabc.Sequence) and not isinstance( - value, str | bytes | bytearray - ): - return _is_non_empty_sequence(value) - message = ( - f"publish setting for package {package_id!r} must be false, a list, or null" - ) - raise WorkspaceModelError(message) - - -def _extract_readme_workspace_flag(package_table: object) -> bool: - """Return ``True`` when ``package_table`` opts into workspace readme.""" - if not isinstance(package_table, cabc.Mapping): - return False - package_mapping = typ.cast("cabc.Mapping[str, typ.Any]", package_table) - readme_value = package_mapping.get("readme") - if not isinstance(readme_value, cabc.Mapping): - return False - readme_mapping = typ.cast("cabc.Mapping[str, typ.Any]", readme_value) - workspace_flag = readme_mapping.get("workspace") - return bool(workspace_flag) - - -def _manifest_uses_workspace_readme(manifest_path: Path) -> bool: - """Return ``True`` when ``readme.workspace`` is set in ``manifest_path``.""" - try: - text = manifest_path.read_text(encoding="utf-8") - except FileNotFoundError as exc: # pragma: no cover - defensive guard - message = f"manifest not found: {manifest_path}" - raise WorkspaceModelError(message) from exc - try: - document = parse(text) - except TOMLKitError as exc: - message = f"failed to parse manifest {manifest_path}: {exc}" - raise WorkspaceModelError(message) from exc - package_table = document.get("package") - return _extract_readme_workspace_flag(package_table) +# The workspace-graph builders live in ``graph_build`` (issue #108) and import +# the model types defined above. To keep the model↔builder boundary acyclic, +# ``models`` no longer imports ``graph_build``; the public construction API is +# re-exported from ``lading.workspace`` (see ``__init__``) instead. +__all__ = [ + "WorkspaceCrate", + "WorkspaceDependency", + "WorkspaceDependencyCycleError", + "WorkspaceGraph", + "WorkspaceIndex", + "WorkspaceModelError", +] diff --git a/tests/bdd/steps/test_bump_steps.py b/tests/bdd/steps/test_bump_steps.py index 6903e383..f429332a 100644 --- a/tests/bdd/steps/test_bump_steps.py +++ b/tests/bdd/steps/test_bump_steps.py @@ -111,8 +111,10 @@ def then_bump_reports_invalid_version( ) -> None: """Assert that invalid versions cause the command to fail with details.""" assert cli_run["returncode"] == 1 - stderr = cli_run["stderr"] - assert f"Invalid version argument '{version}'" in stderr + # Cyclopts renders argument-validation errors through its own console + # (stdout), consistent with other cyclopts errors such as "Unknown command". + stdout = cli_run["stdout"] + assert f"Invalid version argument '{version}'" in stdout @then(parsers.parse('the CLI output lists manifest paths "{first}" and "{second}"')) diff --git a/tests/unit/__snapshots__/test_toml_coerce.ambr b/tests/unit/__snapshots__/test_toml_coerce.ambr new file mode 100644 index 00000000..7ae0fd06 --- /dev/null +++ b/tests/unit/__snapshots__/test_toml_coerce.ambr @@ -0,0 +1,13 @@ +# serializer version: 1 +# name: TestTomlCoerce.test_coercion_error_messages_are_stable + list([ + 'bump.exclude[0] must be a string; received int.', + 'packages[] must be a mapping; received list.', + 'packages must be a sequence; received str.', + 'bump.exclude must be a string or a sequence of strings; received int.', + 'preflight.env must be a TOML table; received int.', + 'preflight.aux_build must be a sequence of string sequences.', + 'bump.rebuild_lockfiles must be a boolean; received int.', + 'preflight.stderr_tail_lines must be an integer; received str.', + ]) +# --- diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 71dcbb23..af253b3e 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -632,7 +632,9 @@ def fail(*_: object, **__: object) -> typ.NoReturn: exit_code = cli.main(["bump", "1.2", "--workspace-root", str(tmp_path)]) assert exit_code == 1 captured = capsys.readouterr() - assert "Invalid version argument '1.2'" in captured.err + # Cyclopts renders argument-validation errors through its own console + # (stdout), consistent with other cyclopts errors such as "Unknown command". + assert "Invalid version argument '1.2'" in captured.out @pytest.mark.usefixtures("minimal_config") diff --git a/tests/unit/test_dependency_sections.py b/tests/unit/test_dependency_sections.py index 37d49e98..ea74a58f 100644 --- a/tests/unit/test_dependency_sections.py +++ b/tests/unit/test_dependency_sections.py @@ -14,7 +14,7 @@ from hypothesis import given, settings from tomlkit import parse as parse_toml -from lading.commands import bump, bump_docs, bump_toml +from lading.commands import bump, bump_docs, bump_manifests, bump_toml if typ.TYPE_CHECKING: from syrupy.assertion import SnapshotAssertion @@ -24,7 +24,7 @@ def test_kind_mapping_agrees_with_canonical_sections() -> None: """Every mapped section is canonical and every canonical section is mapped.""" - mapped = set(bump._DEPENDENCY_SECTION_BY_KIND.values()) + mapped = set(bump_manifests._DEPENDENCY_SECTION_BY_KIND.values()) assert mapped == set(bump_toml.DEPENDENCY_SECTIONS) diff --git a/tests/unit/test_toml_coerce.py b/tests/unit/test_toml_coerce.py new file mode 100644 index 00000000..839a1a90 --- /dev/null +++ b/tests/unit/test_toml_coerce.py @@ -0,0 +1,281 @@ +"""Property and snapshot tests for :mod:`lading.toml_coerce` (issue #108).""" + +from __future__ import annotations + +import dataclasses as dc +import typing as typ + +import hypothesis.strategies as st +import pytest +from hypothesis import given + +from lading import toml_coerce +from lading.config import ConfigurationError +from lading.workspace.models import WorkspaceModelError + +if typ.TYPE_CHECKING: + import collections.abc as cabc + + from syrupy.assertion import SnapshotAssertion + +_ERRORS = (ConfigurationError, WorkspaceModelError) +_error_type = st.sampled_from(_ERRORS) +_non_string = st.one_of(st.integers(), st.booleans(), st.floats(allow_nan=False)) +_strings = st.text(max_size=12) + + +@dc.dataclass(frozen=True, slots=True) +class _IndexedRejectionCase: + """Inputs for asserting an indexed non-string rejection.""" + + values: list[str] + bad_index: int + bad: object + error: type[Exception] + + +def _assert_rejects_indexed_non_string( + coerce: cabc.Callable[..., object], + field_name: str, + case: _IndexedRejectionCase, +) -> None: + """Assert *coerce* rejects a non-string entry, naming its index in the field.""" + position = min(case.bad_index, len(case.values)) + mixed: list[object] = [*case.values] + mixed.insert(position, case.bad) + + with pytest.raises(case.error) as excinfo: + coerce(mixed, field_name, error=case.error) + + assert f"{field_name}[{position}] must be a string" in str(excinfo.value) + assert type(case.bad).__name__ in str(excinfo.value) + + +class TestTomlCoerce: + """Property and snapshot coverage for the coercion helpers.""" + + @given(value=_strings, error=_error_type) + def test_expect_string_passes_strings_through( + self, value: str, error: type[Exception] + ) -> None: + """Well-typed strings pass through unchanged.""" + assert toml_coerce.expect_string(value, "f", error=error) == value + + @given(value=_non_string, error=_error_type) + def test_expect_string_rejects_with_canonical_shape( + self, value: object, error: type[Exception] + ) -> None: + """Ill-typed values raise the bound error with the canonical message.""" + with pytest.raises(error) as excinfo: + toml_coerce.expect_string(value, "demo.field", error=error) + + message = str(excinfo.value) + assert message.startswith("demo.field must be a string; received ") + assert type(value).__name__ in message + + @given(values=st.lists(_strings, max_size=6), error=_error_type) + def test_string_tuple_round_trips_sequences( + self, values: list[str], error: type[Exception] + ) -> None: + """String sequences coerce to equal tuples; None yields empty.""" + assert toml_coerce.string_tuple(values, "f", error=error) == tuple(values) + assert toml_coerce.string_tuple(None, "f", error=error) == () + + @given(values=st.lists(_strings, max_size=6), error=_error_type) + def test_validate_string_sequence_accepts_strings( + self, values: list[str], error: type[Exception] + ) -> None: + """A sequence of only strings returns them as a tuple.""" + assert toml_coerce.validate_string_sequence(values, "f", error=error) == tuple( + values + ) + + @given( + values=st.lists(_strings, max_size=3), + bad_index=st.integers(min_value=0, max_value=3), + bad=_non_string, + error=_error_type, + ) + def test_string_tuple_rejects_non_string_entries( + self, + values: list[str], + bad_index: int, + bad: object, + error: type[Exception], + ) -> None: + """A non-string entry is rejected with its index in the field name.""" + _assert_rejects_indexed_non_string( + toml_coerce.string_tuple, + "demo.list", + _IndexedRejectionCase(values, bad_index, bad, error), + ) + + @given( + values=st.lists(_strings, max_size=3), + bad_index=st.integers(min_value=0, max_value=3), + bad=_non_string, + error=_error_type, + ) + def test_validate_string_sequence_rejects_non_strings( + self, + values: list[str], + bad_index: int, + bad: object, + error: type[Exception], + ) -> None: + """A non-string entry raises with its index in the field name.""" + _assert_rejects_indexed_non_string( + toml_coerce.validate_string_sequence, + "demo.seq", + _IndexedRejectionCase(values, bad_index, bad, error), + ) + + @given(value=st.lists(_strings, max_size=6), error=_error_type) + def test_expect_sequence_accepts_non_string_sequences( + self, value: list[str], error: type[Exception] + ) -> None: + """Non-string sequences pass through unchanged.""" + assert toml_coerce.expect_sequence(value, "f", error=error) == value + + @given(error=_error_type) + def test_expect_sequence_handles_none(self, error: type[Exception]) -> None: + """``allow_none`` returns ``None``; otherwise ``None`` is rejected.""" + assert ( + toml_coerce.expect_sequence(None, "f", error=error, allow_none=True) is None + ) + with pytest.raises(error): + toml_coerce.expect_sequence(None, "f", error=error) + + @given( + value=st.one_of(_strings, st.binary(max_size=6), _non_string), + error=_error_type, + ) + def test_expect_sequence_rejects_strings_and_scalars( + self, value: object, error: type[Exception] + ) -> None: + """Strings, bytes, and scalars raise the bound error type.""" + with pytest.raises(error): + toml_coerce.expect_sequence(value, "demo.field", error=error) + + @given(value=st.lists(st.integers(), min_size=1, max_size=6)) + def test_is_non_empty_sequence_true_for_non_empty(self, value: list[int]) -> None: + """Non-empty non-string sequences are recognised.""" + assert toml_coerce.is_non_empty_sequence(value) is True + + @given( + value=st.one_of( + st.just([]), + st.just(()), + _strings, + st.binary(max_size=6), + _non_string, + ) + ) + def test_is_non_empty_sequence_false_otherwise(self, value: object) -> None: + """Empty sequences, strings, bytes, and scalars are rejected.""" + assert toml_coerce.is_non_empty_sequence(value) is False + + @given( + value=st.lists(st.lists(_strings, max_size=4), max_size=4), + error=_error_type, + ) + def test_string_matrix_round_trips_nested_sequences( + self, value: list[list[str]], error: type[Exception] + ) -> None: + """Nested string sequences coerce to a tuple of tuples; None yields empty.""" + expected = tuple(tuple(row) for row in value) + assert toml_coerce.string_matrix(value, "f", error=error) == expected + assert toml_coerce.string_matrix(None, "f", error=error) == () + + @given(value=st.one_of(_strings, _non_string), error=_error_type) + def test_string_matrix_rejects_non_sequence_values( + self, value: object, error: type[Exception] + ) -> None: + """A scalar or string top-level value raises the bound error type.""" + with pytest.raises(error): + toml_coerce.string_matrix(value, "demo.matrix", error=error) + + @given(bad=_non_string, error=_error_type) + def test_string_matrix_rejects_non_string_rows( + self, bad: object, error: type[Exception] + ) -> None: + """A non-sequence row raises the bound error type.""" + with pytest.raises(error): + toml_coerce.string_matrix([["ok"], bad], "demo.matrix", error=error) + + @given(value=st.one_of(st.none(), st.booleans()), error=_error_type) + def test_boolean_accepts_bools_and_default( + self, + *, + value: bool | None, + error: type[Exception], + ) -> None: + """Booleans pass through; None takes the default.""" + result = toml_coerce.boolean(value, "f", error=error, default=True) + assert result is (True if value is None else value) + + @given(value=st.integers(min_value=0, max_value=999), error=_error_type) + def test_non_negative_int_accepts_valid_values( + self, value: int, error: type[Exception] + ) -> None: + """Non-negative ints and integer strings pass; None takes the default.""" + assert toml_coerce.non_negative_int(value, "f", 7, error=error) == value + # Integer-valued strings still parse (the config string path). + assert toml_coerce.non_negative_int(str(value), "f", 7, error=error) == value + assert toml_coerce.non_negative_int(None, "f", 7, error=error) == 7 + + @given(value=st.integers(max_value=-1), error=_error_type) + def test_non_negative_int_rejects_negative( + self, value: int, error: type[Exception] + ) -> None: + """Negative integers raise the bound error type.""" + with pytest.raises(error, match="must be non-negative"): + toml_coerce.non_negative_int(value, "f", 0, error=error) + + @given( + value=st.one_of( + st.booleans(), + st.floats(allow_nan=False, allow_infinity=False), + ), + error=_error_type, + ) + def test_non_negative_int_rejects_non_integer_types( + self, *, value: bool | float, error: type[Exception] + ) -> None: + """Booleans and floats (e.g. True, 3.9) are rejected, not coerced.""" + with pytest.raises(error, match="must be an integer"): + toml_coerce.non_negative_int(value, "f", 0, error=error) + + @given(error=_error_type) + def test_mapping_helpers_reject_non_mappings(self, error: type[Exception]) -> None: + """Mapping coercers raise the bound error type for non-mappings.""" + with pytest.raises(error): + toml_coerce.expect_mapping([1], "f", error=error) + with pytest.raises(error): + toml_coerce.optional_mapping([1], "f", error=error) + assert toml_coerce.optional_mapping(None, "f", error=error) is None + + def test_coercion_error_messages_are_stable( + self, snapshot: SnapshotAssertion + ) -> None: + """Representative coercion messages change only deliberately.""" + cases: list[str] = [] + for call in ( + lambda: toml_coerce.expect_string(1, "bump.exclude[0]", error=_ERRORS[0]), + lambda: toml_coerce.expect_mapping([], "packages[]", error=_ERRORS[1]), + lambda: toml_coerce.expect_sequence("x", "packages", error=_ERRORS[1]), + lambda: toml_coerce.string_tuple(1, "bump.exclude", error=_ERRORS[0]), + lambda: toml_coerce.string_mapping(1, "preflight.env", error=_ERRORS[0]), + lambda: toml_coerce.string_matrix( + 1, "preflight.aux_build", error=_ERRORS[0] + ), + lambda: toml_coerce.boolean(1, "bump.rebuild_lockfiles", error=_ERRORS[0]), + lambda: toml_coerce.non_negative_int( + "x", "preflight.stderr_tail_lines", 0, error=_ERRORS[0] + ), + ): + with pytest.raises(tuple(_ERRORS)) as excinfo: + call() + cases.append(str(excinfo.value)) + + assert snapshot == cases diff --git a/tests/unit/test_workspace_models_validation.py b/tests/unit/test_workspace_models_validation.py index 0d4d29f5..10973f3e 100644 --- a/tests/unit/test_workspace_models_validation.py +++ b/tests/unit/test_workspace_models_validation.py @@ -7,11 +7,11 @@ import pytest -from lading.workspace import models from tests.helpers.workspace_metadata import build_test_package, create_test_manifest if typ.TYPE_CHECKING: # pragma: no cover - typing helpers only from pathlib import Path +from lading.workspace import _coercion, graph_build, models def test_is_ordering_dependency_skips_unknown_crates() -> None: @@ -28,14 +28,14 @@ def test_is_ordering_dependency_skips_unknown_crates() -> None: def test_build_workspace_graph_requires_workspace_root() -> None: """Missing workspace_root entries should raise an error.""" with pytest.raises(models.WorkspaceModelError, match="workspace_root"): - models.build_workspace_graph({"packages": [], "workspace_members": []}) + graph_build.build_workspace_graph({"packages": [], "workspace_members": []}) def test_index_workspace_packages_skips_non_members() -> None: """Only workspace member packages should be indexed.""" packages = [{"id": "member"}, {"id": "external"}] - index = models._index_workspace_packages(packages, ["member"]) + index = graph_build._index_workspace_packages(packages, ["member"]) assert set(index) == {"member"} @@ -83,9 +83,9 @@ def test_collect_workspace_crates_builds_tuple_in_member_order(tmp_path: Path) - "alpha-id": alpha_package, "beta-id": beta_package, } - workspace_index = models._build_workspace_index(package_lookup) + workspace_index = graph_build._build_workspace_index(package_lookup) - crates = models._collect_workspace_crates( + crates = graph_build._collect_workspace_crates( package_lookup=package_lookup, workspace_member_ids=("beta-id", "alpha-id", "beta-id"), workspace_index=workspace_index, @@ -132,13 +132,13 @@ def test_collect_workspace_crates_rejects_missing_members( package_lookup = { "alpha-id": build_test_package("alpha", "0.1.0", alpha_manifest), } - workspace_index = models._build_workspace_index(package_lookup) + workspace_index = graph_build._build_workspace_index(package_lookup) with pytest.raises( models.WorkspaceModelError, match=r"workspace member 'missing-id' missing from package list", ): - models._collect_workspace_crates( + graph_build._collect_workspace_crates( package_lookup=package_lookup, workspace_member_ids=workspace_member_ids, workspace_index=workspace_index, @@ -150,7 +150,7 @@ def test_build_dependencies_handles_missing_entries() -> None: package = {"id": "crate", "dependencies": None} workspace_index = models.WorkspaceIndex(packages={}, members_by_name={}) - dependencies = models._build_dependencies(package, workspace_index) + dependencies = graph_build._build_dependencies(package, workspace_index) assert dependencies == () @@ -159,17 +159,17 @@ def test_build_dependencies_handles_missing_entries() -> None: ("callable_obj", "args"), [ pytest.param( - models._validate_dependency_mapping, + graph_build._validate_dependency_mapping, ("not-a-mapping",), id="mapping_not_dict", ), pytest.param( - models._validate_dependency_kind, + graph_build._validate_dependency_kind, ({"kind": 123},), id="kind_not_string", ), pytest.param( - models._validate_dependency_kind, + graph_build._validate_dependency_kind, ({"kind": "unknown"},), id="kind_unsupported", ), @@ -186,7 +186,7 @@ def test_dependency_validation_errors( def test_lookup_workspace_target_handles_missing_entries() -> None: """Targets outside the workspace should return None.""" workspace_index = models.WorkspaceIndex(packages={}, members_by_name={}) - result = models._lookup_workspace_target({}, workspace_index) + result = graph_build._lookup_workspace_target({}, workspace_index) assert result is None @@ -194,36 +194,36 @@ def test_lookup_workspace_target_handles_missing_entries() -> None: def test_path_normalisation_rejects_invalid_types() -> None: """Non-path types should be rejected for manifest and root paths.""" with pytest.raises(models.WorkspaceModelError): - models._normalise_workspace_root(123) + graph_build._normalise_workspace_root(123) with pytest.raises(models.WorkspaceModelError): - models._normalise_manifest_path(123, "field") + graph_build._normalise_manifest_path(123, "field") def test_expect_sequence_validation() -> None: """Sequence validation should honour allow_none and reject scalars.""" - assert models._expect_sequence(None, "field", allow_none=True) is None + assert _coercion._expect_sequence(None, "field", allow_none=True) is None with pytest.raises(models.WorkspaceModelError): - models._expect_sequence(None, "field") + _coercion._expect_sequence(None, "field") with pytest.raises(models.WorkspaceModelError): - models._expect_sequence("oops", "field") + _coercion._expect_sequence("oops", "field") def test_expect_string_and_non_empty_sequence_checks() -> None: """String and sequence coercion should reject invalid inputs.""" with pytest.raises(models.WorkspaceModelError): - models._expect_string(123, "field") - assert models._is_non_empty_sequence([]) is False - assert models._is_non_empty_sequence("abc") is False - assert models._is_non_empty_sequence(["a"]) is True + _coercion._expect_string(123, "field") + assert _coercion._is_non_empty_sequence([]) is False + assert _coercion._is_non_empty_sequence("abc") is False + assert _coercion._is_non_empty_sequence(["a"]) is True def test_coerce_publish_setting_allows_sequences_and_bools() -> None: """Publish setting coercion should support bools, lists, and None.""" - assert models._coerce_publish_setting(None, "crate") is True - assert models._coerce_publish_setting(value=False, package_id="crate") is False - assert models._coerce_publish_setting(["crates-io"], "crate") is True + assert graph_build._coerce_publish_setting(None, "crate") is True + assert graph_build._coerce_publish_setting(value=False, package_id="crate") is False + assert graph_build._coerce_publish_setting(["crates-io"], "crate") is True with pytest.raises(models.WorkspaceModelError): - models._coerce_publish_setting("invalid", "crate") + graph_build._coerce_publish_setting("invalid", "crate") def test_topological_sort_dedupes_duplicate_dependencies(tmp_path: Path) -> None: @@ -300,15 +300,15 @@ def test_topological_sort_dedupes_duplicate_dependencies(tmp_path: Path) -> None def test_extract_readme_workspace_flag_handles_non_mappings(tmp_path: Path) -> None: """Non-mapping package tables should return False.""" - assert models._extract_readme_workspace_flag("invalid") is False - assert models._extract_readme_workspace_flag({"readme": "README.md"}) is False + assert graph_build._extract_readme_workspace_flag("invalid") is False + assert graph_build._extract_readme_workspace_flag({"readme": "README.md"}) is False def test_manifest_uses_workspace_readme_detects_flag(tmp_path: Path) -> None: """Manifest helper should detect readme.workspace usage.""" manifest_path = tmp_path / "Cargo.toml" manifest_path.write_text("[package]\nname = 'demo'\nreadme.workspace = true\n") - assert models._manifest_uses_workspace_readme(manifest_path) is True + assert graph_build._manifest_uses_workspace_readme(manifest_path) is True def test_manifest_uses_workspace_readme_reports_parse_errors(tmp_path: Path) -> None: @@ -317,4 +317,4 @@ def test_manifest_uses_workspace_readme_reports_parse_errors(tmp_path: Path) -> manifest_path.write_text("[package\n", encoding="utf-8") with pytest.raises(models.WorkspaceModelError): - models._manifest_uses_workspace_readme(manifest_path) + graph_build._manifest_uses_workspace_readme(manifest_path) From 37b847e5ad93c501b30b062c6f1994fd00160e4c Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 9 Jul 2026 11:14:44 +0200 Subject: [PATCH 2/4] Route coercion errors through _reject; fix stale docstring cross-refs Centralise the canonical error-message shape in the toml_coerce helpers: - `non_negative_int` type/parse-failure arms now raise `_reject(value, field_name, "an integer", error)` (identical message, so the snapshot is unchanged); the `str` arm preserves `from exc` chaining. The negative-value message stays a plain value error (the `"; received {type}."` type-shape does not fit it). - `expect_sequence`'s None-disallowed branch and both `string_matrix` failure arms now route through `_reject`, so they carry the canonical `"; received {type}."` suffix. Regenerated the `preflight.aux_build` line of the `test_coercion_error_messages_are_stable` snapshot accordingly. Docstrings: `_BumpContext` now points at `bump._apply_crate_manifest_update` (the current consumer; `_update_crate_manifest` was removed), and `graph_build`'s module docstring points coercion bindings at `lading.workspace._coercion` rather than `models`. Co-Authored-By: Claude Opus 4.8 (1M context) --- lading/commands/bump_manifests.py | 2 +- lading/toml_coerce/_scalars.py | 7 +++---- lading/toml_coerce/_sequences.py | 8 +++----- lading/workspace/graph_build.py | 7 ++++--- tests/unit/__snapshots__/test_toml_coerce.ambr | 2 +- 5 files changed, 12 insertions(+), 14 deletions(-) diff --git a/lading/commands/bump_manifests.py b/lading/commands/bump_manifests.py index 398d3cc3..e1903358 100644 --- a/lading/commands/bump_manifests.py +++ b/lading/commands/bump_manifests.py @@ -44,7 +44,7 @@ class _BumpContext: """Initialisation context for bump operations. Derived once by ``bump._initialize_bump_context`` and consumed by - :func:`_update_crate_manifest`; the manifest-mutation contract lives with + :func:`bump._apply_crate_manifest_update`; the manifest-mutation contract lives with this extracted module rather than reaching back into ``bump`` internals. """ diff --git a/lading/toml_coerce/_scalars.py b/lading/toml_coerce/_scalars.py index 2599612b..24758813 100644 --- a/lading/toml_coerce/_scalars.py +++ b/lading/toml_coerce/_scalars.py @@ -96,7 +96,6 @@ def non_negative_int( When ``value`` is not a real integer (``bool`` and ``float`` are rejected) or an integer-valued string, or when the result is negative. """ - type_error = f"{field_name} must be an integer; received {type(value).__name__}." # ``bool`` is a subclass of ``int`` and ``float``/other types are truthy for # a blanket ``int(...)`` cast, so dispatch explicitly to accept only real # integers and integer-valued strings (the config string path). @@ -104,16 +103,16 @@ def non_negative_int( case None: return default case bool(): - raise error(type_error) + raise _reject(value, field_name, "an integer", error) case int(): integer = value case str(): try: integer = int(value) except ValueError as exc: - raise error(type_error) from exc + raise _reject(value, field_name, "an integer", error) from exc case _: - raise error(type_error) + raise _reject(value, field_name, "an integer", error) if integer < 0: message = f"{field_name} must be non-negative." raise error(message) diff --git a/lading/toml_coerce/_sequences.py b/lading/toml_coerce/_sequences.py index f282e1e1..af85de11 100644 --- a/lading/toml_coerce/_sequences.py +++ b/lading/toml_coerce/_sequences.py @@ -44,8 +44,7 @@ def expect_sequence( case None: if allow_none: return None - message = f"{field_name} must be a sequence" - raise error(message) + raise _reject(value, field_name, "a sequence", error) case str() | bytes() | bytearray(): raise _reject(value, field_name, "a sequence", error) case cabc.Sequence(): @@ -125,16 +124,15 @@ def string_matrix( value: object, field_name: str, *, error: _ErrorType ) -> tuple[tuple[str, ...], ...]: """Return a tuple-of-tuples parsed from ``value`` as string sequences.""" - message = f"{field_name} must be a sequence of string sequences." match value: case None: return () case str() | bytes(): - raise error(message) + raise _reject(value, field_name, "a sequence of string sequences", error) case cabc.Sequence(): return tuple( _validate_matrix_entry(entry, field_name, index, error) for index, entry in enumerate(value) ) case _: - raise error(message) + raise _reject(value, field_name, "a sequence of string sequences", error) diff --git a/lading/workspace/graph_build.py b/lading/workspace/graph_build.py index b7edc265..a4f34468 100644 --- a/lading/workspace/graph_build.py +++ b/lading/workspace/graph_build.py @@ -2,9 +2,10 @@ Extracted from :mod:`lading.workspace.models` (issue #108) so the data structures and topology stay separate from the metadata-parsing layer. The -module shares the coercion bindings defined in ``models`` so all workspace -validation failures raise :class:`WorkspaceModelError` with the canonical -message shape from :mod:`lading.toml_coerce`. +module shares the coercion bindings defined in +:mod:`lading.workspace._coercion` so all workspace validation failures raise +:class:`WorkspaceModelError` with the canonical message shape from +:mod:`lading.toml_coerce`. """ from __future__ import annotations diff --git a/tests/unit/__snapshots__/test_toml_coerce.ambr b/tests/unit/__snapshots__/test_toml_coerce.ambr index 7ae0fd06..d8e2abd8 100644 --- a/tests/unit/__snapshots__/test_toml_coerce.ambr +++ b/tests/unit/__snapshots__/test_toml_coerce.ambr @@ -6,7 +6,7 @@ 'packages must be a sequence; received str.', 'bump.exclude must be a string or a sequence of strings; received int.', 'preflight.env must be a TOML table; received int.', - 'preflight.aux_build must be a sequence of string sequences.', + 'preflight.aux_build must be a sequence of string sequences; received int.', 'bump.rebuild_lockfiles must be a boolean; received int.', 'preflight.stderr_tail_lines must be an integer; received str.', ]) From 00c3e21d3c469c41f20545b6b5d97e13015f3e17 Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 9 Jul 2026 11:20:39 +0200 Subject: [PATCH 3/4] Lock down canonical coercion-message contracts in tests Address the "canonical-message branches unguarded" review finding by asserting exact error wording (not just the raised type) on the branches that recently moved to `_reject`, and by covering the `expect_mapping` success path: - `test_expect_sequence_handles_none` now asserts the disallowed-None message `"f must be a sequence; received NoneType."`. - `test_string_matrix_rejects_non_sequence_values` asserts the top-level `"... must be a sequence of string sequences; received ."` shape. - `test_string_matrix_rejects_non_string_rows` asserts the row-level `"demo.matrix[1] must be a sequence of strings; received ."` shape. - `test_mapping_helpers_reject_non_mappings` becomes `test_mapping_helpers_accept_and_reject_mappings`, adding a passthrough assertion that a valid mapping is returned unchanged. Tests are strengthened in place (no new methods) to stay within the `TestTomlCoerce` PLR0904 public-method cap. The graph_build module-docstring finding was already fixed in the prior commit. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/unit/test_toml_coerce.py | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/tests/unit/test_toml_coerce.py b/tests/unit/test_toml_coerce.py index 839a1a90..93cba66c 100644 --- a/tests/unit/test_toml_coerce.py +++ b/tests/unit/test_toml_coerce.py @@ -143,8 +143,9 @@ def test_expect_sequence_handles_none(self, error: type[Exception]) -> None: assert ( toml_coerce.expect_sequence(None, "f", error=error, allow_none=True) is None ) - with pytest.raises(error): + with pytest.raises(error) as excinfo: toml_coerce.expect_sequence(None, "f", error=error) + assert str(excinfo.value) == "f must be a sequence; received NoneType." @given( value=st.one_of(_strings, st.binary(max_size=6), _non_string), @@ -192,16 +193,24 @@ def test_string_matrix_rejects_non_sequence_values( self, value: object, error: type[Exception] ) -> None: """A scalar or string top-level value raises the bound error type.""" - with pytest.raises(error): + with pytest.raises(error) as excinfo: toml_coerce.string_matrix(value, "demo.matrix", error=error) + message = str(excinfo.value) + assert ( + "demo.matrix must be a sequence of string sequences; received " in message + ) + assert type(value).__name__ in message @given(bad=_non_string, error=_error_type) def test_string_matrix_rejects_non_string_rows( self, bad: object, error: type[Exception] ) -> None: - """A non-sequence row raises the bound error type.""" - with pytest.raises(error): + """A non-sequence row is rejected, naming its index in the field.""" + with pytest.raises(error) as excinfo: toml_coerce.string_matrix([["ok"], bad], "demo.matrix", error=error) + message = str(excinfo.value) + assert "demo.matrix[1] must be a sequence of strings; received " in message + assert type(bad).__name__ in message @given(value=st.one_of(st.none(), st.booleans()), error=_error_type) def test_boolean_accepts_bools_and_default( @@ -247,8 +256,13 @@ def test_non_negative_int_rejects_non_integer_types( toml_coerce.non_negative_int(value, "f", 0, error=error) @given(error=_error_type) - def test_mapping_helpers_reject_non_mappings(self, error: type[Exception]) -> None: - """Mapping coercers raise the bound error type for non-mappings.""" + def test_mapping_helpers_accept_and_reject_mappings( + self, error: type[Exception] + ) -> None: + """Mapping coercers pass valid mappings through and reject non-mappings.""" + mapping = {"key": "value"} + assert toml_coerce.expect_mapping(mapping, "f", error=error) is mapping + assert toml_coerce.optional_mapping(mapping, "f", error=error) is mapping with pytest.raises(error): toml_coerce.expect_mapping([1], "f", error=error) with pytest.raises(error): From c95c4a9052afde9e31421fa2eb00d9d9f00b3bde Mon Sep 17 00:00:00 2001 From: leynos Date: Thu, 9 Jul 2026 12:25:31 +0200 Subject: [PATCH 4/4] Dedup coercion rejection-assert tail via a shared helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolve the CodeScene "Code Duplication" finding across the four rejection tests. Extract the common raise/capture/assert-substring/assert-type-name tail into a generic `_assert_rejection(call, expected_prefix, bad_value, error)` helper. `_assert_rejects_indexed_non_string` now builds the indexed input and delegates to it, and the two `string_matrix` rejection tests call it directly instead of repeating the tail inline. The `string_tuple` and `validate_string_sequence` rejection tests are unchanged — they already route through `_assert_rejects_indexed_non_string`. All four tests stay distinct, independently discoverable methods; assertion semantics and field-name literals are unchanged. (The helper's summary docstring is wrapped to keep it within the line-length limit.) Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/unit/test_toml_coerce.py | 54 +++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/tests/unit/test_toml_coerce.py b/tests/unit/test_toml_coerce.py index 93cba66c..fa46626b 100644 --- a/tests/unit/test_toml_coerce.py +++ b/tests/unit/test_toml_coerce.py @@ -34,6 +34,24 @@ class _IndexedRejectionCase: error: type[Exception] +def _assert_rejection( + call: cabc.Callable[[], object], + expected_prefix: str, + bad_value: object, + error: type[Exception], +) -> None: + """Assert *call* raises *error* with the expected message shape. + + The message must contain *expected_prefix* and the type name of + *bad_value*. + """ + with pytest.raises(error) as excinfo: + call() + message = str(excinfo.value) + assert expected_prefix in message + assert type(bad_value).__name__ in message + + def _assert_rejects_indexed_non_string( coerce: cabc.Callable[..., object], field_name: str, @@ -43,12 +61,12 @@ def _assert_rejects_indexed_non_string( position = min(case.bad_index, len(case.values)) mixed: list[object] = [*case.values] mixed.insert(position, case.bad) - - with pytest.raises(case.error) as excinfo: - coerce(mixed, field_name, error=case.error) - - assert f"{field_name}[{position}] must be a string" in str(excinfo.value) - assert type(case.bad).__name__ in str(excinfo.value) + _assert_rejection( + lambda: coerce(mixed, field_name, error=case.error), + f"{field_name}[{position}] must be a string", + case.bad, + case.error, + ) class TestTomlCoerce: @@ -193,24 +211,26 @@ def test_string_matrix_rejects_non_sequence_values( self, value: object, error: type[Exception] ) -> None: """A scalar or string top-level value raises the bound error type.""" - with pytest.raises(error) as excinfo: - toml_coerce.string_matrix(value, "demo.matrix", error=error) - message = str(excinfo.value) - assert ( - "demo.matrix must be a sequence of string sequences; received " in message + _assert_rejection( + lambda: toml_coerce.string_matrix(value, "demo.matrix", error=error), + "demo.matrix must be a sequence of string sequences; received ", + value, + error, ) - assert type(value).__name__ in message @given(bad=_non_string, error=_error_type) def test_string_matrix_rejects_non_string_rows( self, bad: object, error: type[Exception] ) -> None: """A non-sequence row is rejected, naming its index in the field.""" - with pytest.raises(error) as excinfo: - toml_coerce.string_matrix([["ok"], bad], "demo.matrix", error=error) - message = str(excinfo.value) - assert "demo.matrix[1] must be a sequence of strings; received " in message - assert type(bad).__name__ in message + _assert_rejection( + lambda: toml_coerce.string_matrix( + [["ok"], bad], "demo.matrix", error=error + ), + "demo.matrix[1] must be a sequence of strings; received ", + bad, + error, + ) @given(value=st.one_of(st.none(), st.booleans()), error=_error_type) def test_boolean_accepts_bools_and_default(