From 72606e3fa6f04b8fcf72c89f6e1732bb895d5a90 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 8 Jul 2026 22:33:59 +0200 Subject: [PATCH 1/9] Promote canonical names in publish_plan and publish_execution (#163) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `publish_plan` defined `_append_section` and `_format_plan` privately and then re-bound them to public aliases; every consumer used the public names. Rename the definitions to `append_section` and `format_plan`, drop the aliases, and remove the underscore variants (plus the internal `_format_crates_section`) from `__all__`. `publish_execution` carried a `split_command` wrapper over `lading.runtime.subprocess_runner.split_command` whose only purpose was mapping `ValueError` to `PublishPreflightError`. Nothing in production code called it — `cmd_mox_runner` uses the runtime helper directly — so remove the wrapper, its alias, and its only test. Refresh the publish-flow diagram in `docs/lading-design.md`, which still showed `split_command` and the cmd-mox helpers under `publish_execution`; those moved to `lading.testing.cmd_mox_runner` in #76. --- docs/lading-design.md | 6 ++---- lading/commands/publish_execution.py | 12 ------------ lading/commands/publish_plan.py | 16 +++++----------- tests/unit/publish/test_command_helpers.py | 9 --------- tests/unit/publish/test_formatting_helpers.py | 2 +- tests/unit/test_publish_formatting.py | 2 +- 6 files changed, 9 insertions(+), 38 deletions(-) diff --git a/docs/lading-design.md b/docs/lading-design.md index 852ca1e5..4ffc4ccc 100644 --- a/docs/lading-design.md +++ b/docs/lading-design.md @@ -207,11 +207,9 @@ graph TD H --> I["Updated staged manifest used for publish"] B --> J["Module: lading.commands.publish_execution (publish_execution.py)"] - J --> K["split_command"] - J --> L["should_use_cmd_mox_stub"] - J --> M["normalise_cmd_mox_command"] + J --> K["_invoke: subprocess execution with error adaptation"] - B --> N["Use re-exported publish helpers"] + B --> N["Module: lading.commands.publish_preflight (publish_preflight.py)"] N --> O["Compose final publish plan and execute commands"] ``` diff --git a/lading/commands/publish_execution.py b/lading/commands/publish_execution.py index 31a4703f..9807dc2c 100644 --- a/lading/commands/publish_execution.py +++ b/lading/commands/publish_execution.py @@ -7,7 +7,6 @@ from lading.commands.publish_errors import PublishPreflightError from lading.runtime import CommandSpawnError -from lading.runtime.subprocess_runner import split_command as _runtime_split_command from lading.runtime.subprocess_runner import ( subprocess_runner as _default_subprocess_runner, ) @@ -36,17 +35,6 @@ def _invoke( raise _publish_error(str(exc)) from exc -def _split_command(command: cabc.Sequence[str]) -> tuple[str, tuple[str, ...]]: - """Return the program and argument tuple for ``command``.""" - try: - return _runtime_split_command(command) - except ValueError as exc: - raise _publish_error(str(exc)) from exc - - -split_command = _split_command - __all__ = [ "_invoke", - "split_command", ] diff --git a/lading/commands/publish_plan.py b/lading/commands/publish_plan.py index 59bc807f..c91ac273 100644 --- a/lading/commands/publish_plan.py +++ b/lading/commands/publish_plan.py @@ -212,7 +212,7 @@ def _format_crates_section( lines.append(empty_message) -def _append_section[T]( +def append_section[T]( lines: list[str], items: cabc.Sequence[T], *, @@ -225,7 +225,7 @@ def _append_section[T]( lines.extend(f"- {formatter(item)}" for item in items) -def _format_plan(plan: PublishPlan, *, strip_patches: StripPatchesSetting) -> str: +def format_plan(plan: PublishPlan, *, strip_patches: StripPatchesSetting) -> str: """Render ``plan`` to a human-readable summary for CLI output.""" lines = [ f"Publish plan for {plan.workspace_root}", @@ -238,19 +238,19 @@ def _format_plan(plan: PublishPlan, *, strip_patches: StripPatchesSetting) -> st header=f"Crates to publish ({len(plan.publishable)}):", empty_message="Crates to publish: none", ) - _append_section( + append_section( lines, plan.skipped_manifest, header="Skipped (publish = false):", formatter=lambda crate: crate.name, ) - _append_section( + append_section( lines, plan.skipped_configuration, header="Skipped via publish.exclude:", formatter=lambda crate: crate.name, ) - _append_section( + append_section( lines, plan.missing_configuration_exclusions, header="Configured exclusions not found in workspace:", @@ -259,15 +259,9 @@ def _format_plan(plan: PublishPlan, *, strip_patches: StripPatchesSetting) -> st return "\n".join(lines) -append_section = _append_section -format_plan = _format_plan - __all__ = [ "PublishPlan", "PublishPlanError", - "_append_section", - "_format_crates_section", - "_format_plan", "append_section", "format_plan", "plan_publication", diff --git a/tests/unit/publish/test_command_helpers.py b/tests/unit/publish/test_command_helpers.py index e426cbf9..e9caeb05 100644 --- a/tests/unit/publish/test_command_helpers.py +++ b/tests/unit/publish/test_command_helpers.py @@ -7,7 +7,6 @@ import pytest from lading import cli -from lading.commands import publish, publish_execution from lading.runtime import subprocess_runner from lading.testing import cmd_mox_runner from lading.testing.cmd_mox_runner import normalise_cmd_mox_command @@ -74,14 +73,6 @@ def test_echo_buffered_output_skips_empty_payloads() -> None: assert sink.getvalue() == "" -def test_split_command_rejects_empty_sequence() -> None: - """Splitting an empty command raises a descriptive error.""" - with pytest.raises(publish.PublishPreflightError) as excinfo: - publish_execution.split_command(()) - - assert "Command sequence must contain" in str(excinfo.value) - - @pytest.mark.parametrize( ("command", "expected_program", "expected_args"), [ diff --git a/tests/unit/publish/test_formatting_helpers.py b/tests/unit/publish/test_formatting_helpers.py index a53167c4..da9e1657 100644 --- a/tests/unit/publish/test_formatting_helpers.py +++ b/tests/unit/publish/test_formatting_helpers.py @@ -51,7 +51,7 @@ def test_append_section_omits_header_for_empty_sequences() -> None: def test_format_plan_formats_skipped_sections(tmp_path: Path) -> None: - """``_format_plan`` renders skipped crates using their names only.""" + """``format_plan`` renders skipped crates using their names only.""" root = tmp_path.resolve() manifest_skipped = make_crate(root, "beta", publish_flag=False) config_skipped = make_crate(root, "gamma") diff --git a/tests/unit/test_publish_formatting.py b/tests/unit/test_publish_formatting.py index c253a174..0b839c4f 100644 --- a/tests/unit/test_publish_formatting.py +++ b/tests/unit/test_publish_formatting.py @@ -56,7 +56,7 @@ def test_format_plan_formats_skipped_sections( tmp_path: Path, make_crate: cabc.Callable[[Path, str, _CrateSpec | None], WorkspaceCrate], ) -> None: - """``_format_plan`` renders skipped crates using their names only.""" + """``format_plan`` renders skipped crates using their names only.""" root = tmp_path.resolve() manifest_skipped = make_crate(root, "beta", _CrateSpec(publish=False)) config_skipped = make_crate(root, "gamma") From 369dfc178c7fcf2349f2242908f549b8d0ba4051 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 8 Jul 2026 22:34:12 +0200 Subject: [PATCH 2/9] Remove preflight compatibility shims from publish (#163) Delete the backwards-compatibility layer that `publish.py` kept for test patches after the pre-flight helpers moved to `publish_preflight` (#96, #123): - Drop the eleven bare aliases (`_preflight_argument_sets`, `_CargoPreflightOptions`, `_run_cargo_preflight`, and friends). - Drop the thin `_run_preflight_checks` wrapper that resolved an optional `configuration`; `run()` already resolves configuration before the call, so it now invokes `publish_preflight._run_preflight_checks` directly. - Drop the unused `_append_section`, `_format_plan`, `metadata_module`, `StripPatchesSetting`, and `PublishPlanError` re-bindings. Private symbols carry no stability contract and lading is the only consumer of its own internals, so tests now patch and call the canonical module: conftest fixtures stub `publish_preflight._run_preflight_checks`, the preflight suites target `publish_preflight` directly, and the plan-validation tests raise `publish_plan.PublishPlanError`. The two tests that pinned the wrapper's aliasing behaviour are removed with the wrapper. --- lading/commands/publish.py | 64 ++------------ tests/unit/conftest.py | 6 +- tests/unit/publish/conftest.py | 6 +- tests/unit/publish/preflight_test_utils.py | 4 +- tests/unit/publish/test_plan_validation.py | 8 +- .../publish/test_preflight_cargo_runner.py | 44 +++++----- tests/unit/publish/test_preflight_checks.py | 86 +++++-------------- tests/unit/publish/test_run_preflight.py | 8 +- tests/unit/test_publish_planning.py | 8 +- 9 files changed, 69 insertions(+), 165 deletions(-) diff --git a/lading/commands/publish.py b/lading/commands/publish.py index da5b2fc2..0d253779 100644 --- a/lading/commands/publish.py +++ b/lading/commands/publish.py @@ -36,12 +36,10 @@ **Related modules** * :mod:`lading.commands.publish_plan` — plan construction and formatting -* :mod:`lading.commands.publish_preflight` — canonical home of - ``_run_preflight_checks`` and ``_preflight_argument_sets`` (``cargo check`` - / ``cargo test`` / git-status guards). This module re-exports - ``_preflight_argument_sets`` as a bare alias and ``_run_preflight_checks`` - as a thin configuration-resolving wrapper, both for backwards - compatibility with existing test patches. +* :mod:`lading.commands.publish_preflight` — canonical home of the publish + pre-flight checks (``cargo check`` / ``cargo test`` / git-status guards). + Callers and tests use that module directly; this module holds no + compatibility aliases for its helpers. * :mod:`lading.commands.publish_errors` — :class:`PublishPreflightError` and :class:`PublishError` * :mod:`lading.commands.publish_execution` — subprocess invocation and cmd-mox @@ -59,7 +57,7 @@ from pathlib import Path from lading import config as config_module -from lading.commands import publish_preflight as _publish_preflight +from lading.commands import publish_preflight from lading.commands.cargo_output_adapter import ( CargoIndexLookupFailure, CargoSubprocessResult, @@ -82,38 +80,10 @@ ) from lading.commands.publish_plan import ( PublishPlan, - append_section, format_plan, plan_publication, ) -from lading.commands.publish_plan import ( - PublishPlanError as _PublishPlanError, -) from lading.utils.path import normalise_workspace_root -from lading.workspace import metadata as _metadata_module - -StripPatchesSetting = config_module.StripPatchesSetting -metadata_module = _metadata_module -PublishPlanError = _PublishPlanError -_append_section = append_section -_format_plan = format_plan -# Backwards-compatible aliases (issue #96): publish_preflight owns the -# canonical implementations; existing tests patch and call them through -# this module, so the names must keep resolving here. Plain assignments -# (not imports) keep the re-export intent visible to linters. -# (``_run_preflight_checks`` is the exception: it is a thin wrapper defined -# below that preserves the historical optional-``configuration`` contract.) -_preflight_argument_sets = _publish_preflight._preflight_argument_sets -_CargoPreflightOptions = _publish_preflight._CargoPreflightOptions -_apply_compiletest_externs = _publish_preflight._apply_compiletest_externs -_build_preflight_environment = _publish_preflight._build_preflight_environment -_build_test_arguments = _publish_preflight._build_test_arguments -_compose_preflight_arguments = _publish_preflight._compose_preflight_arguments -_normalise_test_excludes = _publish_preflight._normalise_test_excludes -_run_aux_build_commands = _publish_preflight._run_aux_build_commands -_run_cargo_preflight = _publish_preflight._run_cargo_preflight -_validate_lockfile_freshness = _publish_preflight._validate_lockfile_freshness -_verify_clean_working_tree = _publish_preflight._verify_clean_working_tree LOGGER = logging.getLogger(__name__) @@ -592,28 +562,6 @@ def _ensure_configuration( return config_module.load_configuration(workspace_root) -def _run_preflight_checks( - workspace_root: Path, - *, - allow_dirty: bool, - configuration: LadingConfig | None = None, - runner: CommandRunner | None = None, -) -> None: - """Run publish pre-flight checks, resolving configuration when absent. - - Thin wrapper over :func:`publish_preflight._run_preflight_checks` that - preserves the historical optional-``configuration`` contract: callers may - omit ``configuration`` and have it loaded from the active context or the - workspace before the canonical implementation runs. - """ - _publish_preflight._run_preflight_checks( - workspace_root, - allow_dirty=allow_dirty, - configuration=_ensure_configuration(configuration, workspace_root), - runner=runner, - ) - - def _ensure_workspace( workspace: WorkspaceGraph | None, workspace_root: Path ) -> WorkspaceGraph: @@ -705,7 +653,7 @@ def run( active_configuration = _ensure_configuration(configuration_override, root_path) active_workspace = _ensure_workspace(workspace_override, root_path) - _run_preflight_checks( + publish_preflight._run_preflight_checks( root_path, allow_dirty=effective_options.allow_dirty, configuration=active_configuration, diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 716e9e38..7de50610 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -16,11 +16,9 @@ import tomlkit from lading import config as config_module -from lading.commands import bump, publish +from lading.commands import bump, publish, publish_preflight from lading.workspace import WorkspaceCrate, WorkspaceDependency, WorkspaceGraph -_ORIGINAL_PREFLIGHT = publish._run_preflight_checks - # These modules drive ``bump.run`` to exercise manifest updates, documentation # rewriting, and the rebuild_lockfiles resolution logic -- none of which need # Cargo to actually build or regenerate lockfiles. The stub is scoped to them by @@ -69,7 +67,7 @@ class PublishFixtures: def disable_publish_preflight(monkeypatch: pytest.MonkeyPatch) -> None: """Stub publish pre-flight checks for tests that do not exercise them.""" monkeypatch.setattr( - publish, + publish_preflight, "_run_preflight_checks", lambda *_args, **_kwargs: None, ) diff --git a/tests/unit/publish/conftest.py b/tests/unit/publish/conftest.py index 18d41b29..5cd2836e 100644 --- a/tests/unit/publish/conftest.py +++ b/tests/unit/publish/conftest.py @@ -10,7 +10,7 @@ import pytest from lading import config as config_module -from lading.commands import publish +from lading.commands import publish, publish_preflight from lading.workspace import WorkspaceCrate, WorkspaceDependency, WorkspaceGraph if typ.TYPE_CHECKING: @@ -261,14 +261,14 @@ def publish_plan_and_prep( ORIGINAL_INVOKE = publish._invoke -ORIGINAL_PREFLIGHT = publish._run_preflight_checks +ORIGINAL_PREFLIGHT = publish_preflight._run_preflight_checks @pytest.fixture(autouse=True) def disable_preflight(monkeypatch: pytest.MonkeyPatch) -> None: """Stub publish pre-flight checks for tests unless explicitly restored.""" monkeypatch.setattr( - publish, "_run_preflight_checks", lambda *_args, **_kwargs: None + publish_preflight, "_run_preflight_checks", lambda *_args, **_kwargs: None ) monkeypatch.setattr( publish, diff --git a/tests/unit/publish/preflight_test_utils.py b/tests/unit/publish/preflight_test_utils.py index 51eaa921..878f3b47 100644 --- a/tests/unit/publish/preflight_test_utils.py +++ b/tests/unit/publish/preflight_test_utils.py @@ -6,7 +6,7 @@ import typing as typ from pathlib import Path -from lading.commands import publish +from lading.commands import publish, publish_preflight from .conftest import ORIGINAL_PREFLIGHT, make_crate, make_workspace @@ -27,7 +27,7 @@ def _setup_preflight_test( crate_names: cabc.Sequence[str] | None = None, ) -> tuple[Path, WorkspaceGraph, RecordedCommands]: """Execute ``publish.run`` with optional workspace crates and capture calls.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) root = tmp_path / "workspace" root.mkdir() selected_crates = ("alpha",) if crate_names is None else tuple(crate_names) diff --git a/tests/unit/publish/test_plan_validation.py b/tests/unit/publish/test_plan_validation.py index d61f789a..ba17bdb2 100644 --- a/tests/unit/publish/test_plan_validation.py +++ b/tests/unit/publish/test_plan_validation.py @@ -6,7 +6,7 @@ import pytest -from lading.commands import publish +from lading.commands import publish, publish_plan from .conftest import ( make_config, @@ -29,7 +29,7 @@ def test_plan_publication_rejects_incomplete_configured_order(tmp_path: Path) -> workspace = make_workspace(root, alpha, beta) configuration = make_config(order=("alpha",)) - with pytest.raises(publish.PublishPlanError) as excinfo: + with pytest.raises(publish_plan.PublishPlanError) as excinfo: publish.plan_publication(workspace, configuration) message = str(excinfo.value) @@ -41,7 +41,7 @@ def test_plan_publication_rejects_unknown_configured_crates(tmp_path: Path) -> N """Names outside the publishable set trigger an informative error.""" alpha, _, _ = make_dependency_chain(tmp_path.resolve()) - with pytest.raises(publish.PublishPlanError) as excinfo: + with pytest.raises(publish_plan.PublishPlanError) as excinfo: plan_with_crates(tmp_path, (alpha,), order=("alpha", "omega")) assert "publish.order references crates outside the publishable set" in str( @@ -57,7 +57,7 @@ def test_plan_publication_detects_dependency_cycles(tmp_path: Path) -> None: workspace = make_workspace(root, alpha, beta) configuration = make_config() - with pytest.raises(publish.PublishPlanError) as excinfo: + with pytest.raises(publish_plan.PublishPlanError) as excinfo: publish.plan_publication(workspace, configuration) assert "dependency cycle" in str(excinfo.value) diff --git a/tests/unit/publish/test_preflight_cargo_runner.py b/tests/unit/publish/test_preflight_cargo_runner.py index a918e003..c3f58c08 100644 --- a/tests/unit/publish/test_preflight_cargo_runner.py +++ b/tests/unit/publish/test_preflight_cargo_runner.py @@ -9,7 +9,7 @@ import pytest -from lading.commands import publish +from lading.commands import publish_preflight from .conftest import ORIGINAL_PREFLIGHT @@ -32,12 +32,14 @@ def failing_runner( assert command[0] == "cargo" return 1, "", "boom" - with pytest.raises(publish.PublishPreflightError) as excinfo: - publish._run_cargo_preflight( + with pytest.raises(publish_preflight.PublishPreflightError) as excinfo: + publish_preflight._run_cargo_preflight( tmp_path, "check", runner=failing_runner, - options=publish._CargoPreflightOptions(extra_args=("--workspace",)), + options=publish_preflight._CargoPreflightOptions( + extra_args=("--workspace",) + ), ) message = str(excinfo.value) @@ -87,12 +89,14 @@ def failing_runner( del command, cwd, env return case.exit_code, "", case.stderr - with pytest.raises(publish.PublishPreflightError) as excinfo: - publish._run_cargo_preflight( + with pytest.raises(publish_preflight.PublishPreflightError) as excinfo: + publish_preflight._run_cargo_preflight( tmp_path, case.subcommand, runner=failing_runner, - options=publish._CargoPreflightOptions(extra_args=("--workspace",)), + options=publish_preflight._CargoPreflightOptions( + extra_args=("--workspace",) + ), ) assert str(excinfo.value) == snapshot() @@ -102,19 +106,19 @@ def failing_runner( class _RunCargoPreflightCase: """Parameters for a single cargo-preflight argument-construction scenario.""" - options: publish._CargoPreflightOptions + options: publish_preflight._CargoPreflightOptions expected_tail: tuple[str, ...] def _run_and_record_cargo_preflight( workspace_root: Path, subcommand: typ.Literal["check", "test"], - options: publish._CargoPreflightOptions, + options: publish_preflight._CargoPreflightOptions, ) -> tuple[str, ...]: """Run cargo preflight through a recording runner. The helper injects a closure named ``recording_runner`` into - ``publish._run_cargo_preflight``. That closure captures the subprocess + ``publish_preflight._run_cargo_preflight``. That closure captures the subprocess arguments instead of executing Cargo, so tests can assert the constructed command returned as a tuple. @@ -133,7 +137,7 @@ def recording_runner( recorded.append(command) return 0, "", "" - publish._run_cargo_preflight( + publish_preflight._run_cargo_preflight( workspace_root, subcommand, runner=recording_runner, @@ -149,7 +153,7 @@ def recording_runner( [ pytest.param( _RunCargoPreflightCase( - options=publish._CargoPreflightOptions( + options=publish_preflight._CargoPreflightOptions( extra_args=("--workspace", "--all-targets"), test_excludes=(" alpha ", "", "beta"), ), @@ -159,7 +163,7 @@ def recording_runner( ), pytest.param( _RunCargoPreflightCase( - options=publish._CargoPreflightOptions( + options=publish_preflight._CargoPreflightOptions( extra_args=("--workspace", "--all-targets"), test_excludes=["", " ", "\t", "\n"], ), @@ -169,7 +173,7 @@ def recording_runner( ), pytest.param( _RunCargoPreflightCase( - options=publish._CargoPreflightOptions( + options=publish_preflight._CargoPreflightOptions( extra_args=("--workspace", "--all-targets"), unit_tests_only=True, ), @@ -179,7 +183,7 @@ def recording_runner( ), pytest.param( _RunCargoPreflightCase( - options=publish._CargoPreflightOptions( + options=publish_preflight._CargoPreflightOptions( extra_args=("--workspace", "--all-targets"), test_excludes=["slow-integration"], unit_tests_only=True, @@ -195,7 +199,7 @@ def recording_runner( ), pytest.param( _RunCargoPreflightCase( - options=publish._CargoPreflightOptions( + options=publish_preflight._CargoPreflightOptions( extra_args=("--workspace", "--all-targets"), unit_tests_only=False, ), @@ -222,7 +226,7 @@ def test_compiletest_diagnostic_details( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """Failing cargo test pre-flight lists stderr artifacts with tail output.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) artifact = tmp_path / "ui.stderr" artifact.write_text("line1\nline2\nline3\n", encoding="utf-8") @@ -234,13 +238,13 @@ def failing_runner( ) -> tuple[int, str, str]: return 1, f"diff at {artifact}", "" - options = publish._CargoPreflightOptions( + options = publish_preflight._CargoPreflightOptions( extra_args=("--workspace",), env={}, diagnostics_tail_lines=2, ) - with pytest.raises(publish.PublishPreflightError) as excinfo: - publish._run_cargo_preflight( + with pytest.raises(publish_preflight.PublishPreflightError) as excinfo: + publish_preflight._run_cargo_preflight( tmp_path, "test", runner=failing_runner, diff --git a/tests/unit/publish/test_preflight_checks.py b/tests/unit/publish/test_preflight_checks.py index a1b15f69..cfde892e 100644 --- a/tests/unit/publish/test_preflight_checks.py +++ b/tests/unit/publish/test_preflight_checks.py @@ -8,71 +8,21 @@ import pytest -from lading.commands import publish, publish_preflight +from lading.commands import publish_preflight from .conftest import ORIGINAL_PREFLIGHT, make_config, make_preflight_config if typ.TYPE_CHECKING: from syrupy.assertion import SnapshotAssertion - from lading.config import LadingConfig from lading.runtime import CommandRunner -def test_publish_preflight_aliases_are_wired_correctly() -> None: - """Backwards-compatible preflight names keep resolving into publish_preflight. - - ``_preflight_argument_sets`` is a bare re-export, so identity must hold. - ``_run_preflight_checks`` is intentionally a thin wrapper (issue #96) that - preserves the optional-``configuration`` contract, so it must *not* be the - canonical object; its delegation is pinned by - ``test_preflight_wrapper_loads_configuration_when_omitted``. - """ - assert ( - publish._preflight_argument_sets is publish_preflight._preflight_argument_sets - ) - assert publish._run_preflight_checks is not publish_preflight._run_preflight_checks - - -def test_preflight_wrapper_loads_configuration_when_omitted( - monkeypatch: pytest.MonkeyPatch, tmp_path: Path -) -> None: - """Omitting ``configuration`` resolves it before delegating to canonical.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) - configuration = make_config() - monkeypatch.setattr( - publish.config_module, "current_configuration", lambda: configuration - ) - recorded: dict[str, typ.Any] = {} - - def recording_preflight( - workspace_root: Path, - *, - allow_dirty: bool, - configuration: LadingConfig, - runner: CommandRunner | None = None, - ) -> None: - recorded["workspace_root"] = workspace_root - recorded["allow_dirty"] = allow_dirty - recorded["configuration"] = configuration - - monkeypatch.setattr(publish_preflight, "_run_preflight_checks", recording_preflight) - - root = tmp_path / "workspace" - root.mkdir() - - publish._run_preflight_checks(root, allow_dirty=True) - - assert recorded["configuration"] is configuration - assert recorded["workspace_root"] == root - assert recorded["allow_dirty"] is True - - def test_preflight_checks_remove_all_targets_for_unit_only( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """Unit-test-only mode omits --all-targets from cargo test pre-flight.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) monkeypatch.setattr( publish_preflight, "_verify_clean_working_tree", lambda *_args, **_kwargs: None ) @@ -93,7 +43,9 @@ def recording_preflight( root.mkdir() configuration = make_config(preflight=make_preflight_config(unit_tests_only=True)) - publish._run_preflight_checks(root, allow_dirty=False, configuration=configuration) + publish_preflight._run_preflight_checks( + root, allow_dirty=False, configuration=configuration + ) assert set(recorded) == {"check", "test"} check_args = recorded["check"].extra_args @@ -111,7 +63,7 @@ def test_preflight_checks_support_special_target_dir( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """Target directories with spaces/symbols propagate without quoting issues.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) monkeypatch.setattr( publish_preflight, "_verify_clean_working_tree", lambda *_args, **_kwargs: None ) @@ -148,7 +100,9 @@ def __exit__(self, *_args: object) -> bool: root.mkdir() configuration = make_config() - publish._run_preflight_checks(root, allow_dirty=False, configuration=configuration) + publish_preflight._run_preflight_checks( + root, allow_dirty=False, configuration=configuration + ) assert set(recorded) == {"check", "test"} for args in recorded.values(): @@ -161,7 +115,7 @@ def test_preflight_runs_aux_build_commands( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """Auxiliary build commands execute before cargo pre-flight calls.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) root = tmp_path / "workspace" root.mkdir() commands: list[tuple[tuple[str, ...], Path | None]] = [] @@ -182,7 +136,7 @@ def recording_runner( preflight=make_preflight_config(aux_build=(("cargo", "test", "-p", "lint"),)) ) - publish._run_preflight_checks( + publish_preflight._run_preflight_checks( root, allow_dirty=True, configuration=configuration, @@ -199,7 +153,7 @@ def test_aux_build_failure_surfaces_error( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """Failures in aux build commands abort pre-flight with context.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) root = tmp_path / "workspace" root.mkdir() @@ -224,8 +178,8 @@ def runner( ) ) - with pytest.raises(publish.PublishPreflightError) as excinfo: - publish._run_preflight_checks( + with pytest.raises(publish_preflight.PublishPreflightError) as excinfo: + publish_preflight._run_preflight_checks( root, allow_dirty=True, configuration=configuration, @@ -239,7 +193,7 @@ def test_preflight_env_overrides_forwarded( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """Environment overrides propagate to cargo pre-flight invocations.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) root = tmp_path / "workspace" root.mkdir() captured_env: dict[str, str] = {} @@ -261,7 +215,7 @@ def env_recording_runner( preflight=make_preflight_config(env_overrides=(("DYLINT_LOCALE", "cy"),)) ) - publish._run_preflight_checks( + publish_preflight._run_preflight_checks( root, allow_dirty=True, configuration=configuration, @@ -275,7 +229,7 @@ def test_preflight_append_compiletest_externs( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """Compiletest externs extend RUSTFLAGS for cargo test.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) root = tmp_path / "workspace" root.mkdir() artifact = root / "target" / "lint" / "liblint_macro.so" @@ -309,7 +263,7 @@ def recording_runner( ) ) - publish._run_preflight_checks( + publish_preflight._run_preflight_checks( root, allow_dirty=True, configuration=configuration, @@ -337,7 +291,7 @@ def dirty_runner( assert cwd == root return 0, " M file\n", "" - with pytest.raises(publish.PublishPreflightError) as excinfo: + with pytest.raises(publish_preflight.PublishPreflightError) as excinfo: publish_preflight._verify_clean_working_tree( root, allow_dirty=False, runner=dirty_runner ) @@ -369,7 +323,7 @@ def missing_runner( assert cwd == tmp_path return 128, "", "fatal: Not a git repository" - with pytest.raises(publish.PublishPreflightError) as excinfo: + with pytest.raises(publish_preflight.PublishPreflightError) as excinfo: publish_preflight._verify_clean_working_tree( tmp_path, allow_dirty=False, runner=missing_runner ) diff --git a/tests/unit/publish/test_run_preflight.py b/tests/unit/publish/test_run_preflight.py index c973619a..54241526 100644 --- a/tests/unit/publish/test_run_preflight.py +++ b/tests/unit/publish/test_run_preflight.py @@ -59,7 +59,7 @@ def test_run_executes_preflight_checks_in_workspace( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """Pre-flight commands run inside the resolved workspace root.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) root = tmp_path / "workspace" root.mkdir() workspace = make_workspace(root, make_crate(root, "alpha")) @@ -248,7 +248,7 @@ def test_dirty_workspace_allowed_by_default( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """Publish skips git status when cleanliness enforcement is disabled.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) root = tmp_path / "workspace" root.mkdir() workspace = make_workspace(root, make_crate(root, "alpha")) @@ -280,7 +280,7 @@ def test_forbid_dirty_flag_enforces_cleanliness( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: """Explicit forbid-dirty option requires a clean git status.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) root = tmp_path / "workspace" root.mkdir() workspace = make_workspace(root, make_crate(root, "alpha")) @@ -327,7 +327,7 @@ def test_run_raises_when_preflight_cargo_fails( expected_message: str, ) -> None: """Non-zero cargo check/test aborts the publish command.""" - monkeypatch.setattr(publish, "_run_preflight_checks", ORIGINAL_PREFLIGHT) + monkeypatch.setattr(publish_preflight, "_run_preflight_checks", ORIGINAL_PREFLIGHT) root = tmp_path / "workspace" root.mkdir() workspace = make_workspace(root, make_crate(root, "alpha")) diff --git a/tests/unit/test_publish_planning.py b/tests/unit/test_publish_planning.py index b58c7721..8e6c9383 100644 --- a/tests/unit/test_publish_planning.py +++ b/tests/unit/test_publish_planning.py @@ -7,7 +7,7 @@ import pytest -from lading.commands import publish +from lading.commands import publish, publish_plan from tests.unit.conftest import PlanningFixtures, _CrateSpec if typ.TYPE_CHECKING: @@ -314,7 +314,7 @@ def test_plan_publication_detects_dependency_cycles( name_b="beta", ) - with pytest.raises(publish.PublishPlanError) as excinfo: + with pytest.raises(publish_plan.PublishPlanError) as excinfo: _plan_with_crates( planning_fixtures.tmp_path, planning_fixtures.make_workspace, @@ -398,7 +398,7 @@ def test_plan_publication_rejects_incomplete_configured_order( workspace = fx.make_workspace(root, alpha, beta) configuration = fx.make_config(order=("alpha",)) - with pytest.raises(publish.PublishPlanError) as excinfo: + with pytest.raises(publish_plan.PublishPlanError) as excinfo: publish.plan_publication(workspace, configuration) message = str(excinfo.value) @@ -434,7 +434,7 @@ def test_plan_publication_order_validation_errors( make_dependency=fx.make_dependency, ) - with pytest.raises(publish.PublishPlanError) as excinfo: + with pytest.raises(publish_plan.PublishPlanError) as excinfo: _plan_with_crates( fx.tmp_path, fx.make_workspace, From ecd943f3871a149168fac0741554cc74a27777f2 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 8 Jul 2026 22:34:21 +0200 Subject: [PATCH 3/9] Remove bump_toml compatibility aliases from bump (#163) Drop the six module-level re-exports (`_parse_manifest`, `_select_table`, `_assign_version`, `_value_matches`, `_update_dependency_sections`, `_update_dependency_table`) that `bump.py` kept solely so existing tests could keep patching the old names. The TOML helper tests now exercise `bump_toml` directly. Also fold the redundant `_log = LOGGER` alias into a single `LOGGER` name; the README-transposition failure path now logs via `LOGGER.exception`, matching the logging convention enforced by lint for handlers that re-raise. --- lading/commands/bump.py | 32 ++++++++---------------- tests/unit/test_bump_manifest_writing.py | 8 +++--- tests/unit/test_bump_toml_helpers.py | 28 ++++++++++----------- 3 files changed, 28 insertions(+), 40 deletions(-) diff --git a/lading/commands/bump.py b/lading/commands/bump.py index eed78ef6..456d0566 100644 --- a/lading/commands/bump.py +++ b/lading/commands/bump.py @@ -24,7 +24,6 @@ - :mod:`lading.commands.bump_docs` — documentation version rewrites. - :mod:`lading.commands.bump_lockfiles` — lockfile regeneration. - :mod:`lading.commands.bump_readme` — workspace README transposition. -- :mod:`lading.commands.bump_toml` — low-level TOML manipulation. """ from __future__ import annotations @@ -42,7 +41,6 @@ bump_lockfiles, bump_manifests, bump_readme, - bump_toml, ) from lading.commands.bump_manifests import ( _WORKSPACE_SELECTORS, @@ -64,7 +62,6 @@ from lading.workspace import WorkspaceCrate, WorkspaceGraph LOGGER = logging.getLogger(__name__) -_log = LOGGER @dc.dataclass(frozen=True, slots=True) @@ -134,7 +131,7 @@ def run( ) -> str: """Update workspace and crate manifest versions to ``target_version``.""" context = _initialize_bump_context(workspace_root, options) - _log.debug( + LOGGER.debug( "Bump context initialised: %d excluded crate(s), %d to update", len(context.excluded), len(context.updated_crate_names), @@ -182,7 +179,7 @@ def _initialize_bump_context( if resolved_options.rebuild_lockfiles is None else resolved_options.rebuild_lockfiles ) - _log.debug( + LOGGER.debug( "rebuild_lockfiles resolution: raw_flag=%r, configured_default=%r, resolved=%r", resolved_options.rebuild_lockfiles, configuration.bump.rebuild_lockfiles, @@ -253,7 +250,7 @@ def _process_crate_manifests( updated_count += 1 case _CrateManifestOutcome.UNCHANGED: pass - _log.debug( + LOGGER.debug( "Crate manifest processing complete: %d processed, %d skipped, %d updated", processed_count, skipped_count, @@ -279,7 +276,7 @@ def _process_documentation_files( def _process_readme_transposition(context: _BumpContext, *, dry_run: bool) -> set[Path]: """Transpose workspace README files into opted-in member crates.""" - _log.debug("Starting workspace README transposition") + LOGGER.debug("Starting workspace README transposition") changed_readmes: set[Path] = set() transposed_entry_count = 0 source_readme_path = context.root_path / "README.md" @@ -300,11 +297,11 @@ def _process_readme_transposition(context: _BumpContext, *, dry_run: bool) -> se _source_text=cached_text, ) except bump_readme.ReadmeTranspositionError: - _log.error("README transposition failed for crate %r", crate.name) + LOGGER.exception("README transposition failed for crate %r", crate.name) raise if changed_path is not None: changed_readmes.add(changed_path) - _log.debug( + LOGGER.debug( "README transposition complete: %d entries, %d file(s) changed", transposed_entry_count, len(changed_readmes), @@ -379,7 +376,7 @@ def _apply_crate_manifest_update( ) if _should_skip_crate_update(selectors, dependency_sections): - _log.debug("Skipping crate manifest update for excluded crate %r", crate.name) + LOGGER.debug("Skipping crate manifest update for excluded crate %r", crate.name) return _CrateManifestOutcome.SKIPPED crate_options = dc.replace( @@ -393,19 +390,19 @@ def _apply_crate_manifest_update( crate_options, ) if was_updated: - _log.debug( + LOGGER.debug( "Updated crate manifest for crate %r: manifest=%s", crate.name, crate.manifest_path, ) if not crate_options.dry_run: - _log.debug( + LOGGER.debug( "Wrote crate manifest for crate %r: manifest=%s", crate.name, crate.manifest_path, ) else: - _log.debug( + LOGGER.debug( "Crate manifest already up to date for crate %r: manifest=%s", crate.name, crate.manifest_path, @@ -415,12 +412,3 @@ def _apply_crate_manifest_update( if was_updated else _CrateManifestOutcome.UNCHANGED ) - - -# 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 -_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/tests/unit/test_bump_manifest_writing.py b/tests/unit/test_bump_manifest_writing.py index d47fadae..6406c24b 100644 --- a/tests/unit/test_bump_manifest_writing.py +++ b/tests/unit/test_bump_manifest_writing.py @@ -7,7 +7,7 @@ import pytest from tomlkit import parse as parse_toml -from lading.commands import bump +from lading.commands import bump, bump_toml from tests.helpers.workspace_builders import _load_version @@ -71,7 +71,7 @@ def test_update_dependency_sections_workspace_flag( '[dependencies]\nalpha = "0.1.0"\n\n' '[workspace.dependencies]\nalpha = "^0.1.0"\n' ) - changed = bump._update_dependency_sections( + changed = bump_toml.update_dependency_sections( document, {"dependencies": ("alpha",)}, "1.0.0", @@ -89,7 +89,7 @@ def test_update_dependency_sections_workspace_flag( def test_update_dependency_sections_workspace_only() -> None: """When only workspace sections exist, they are updated with the flag.""" document = parse_toml('[workspace.dependencies]\nalpha = "0.1.0"\n') - changed = bump._update_dependency_sections( + changed = bump_toml.update_dependency_sections( document, {"dependencies": ("alpha",)}, "2.0.0", @@ -107,7 +107,7 @@ def test_update_dependency_sections_workspace_dev_and_build() -> None: '[workspace.dev-dependencies]\nalpha = "~0.1.0"\n\n' '[workspace.build-dependencies]\nbeta = { version = "0.1.0" }\n' ) - changed = bump._update_dependency_sections( + changed = bump_toml.update_dependency_sections( document, {"dev-dependencies": ("alpha",), "build-dependencies": ("beta",)}, "3.0.0", diff --git a/tests/unit/test_bump_toml_helpers.py b/tests/unit/test_bump_toml_helpers.py index db2e006e..2231548f 100644 --- a/tests/unit/test_bump_toml_helpers.py +++ b/tests/unit/test_bump_toml_helpers.py @@ -4,13 +4,13 @@ from tomlkit import parse as parse_toml -from lading.commands import bump +from lading.commands import bump_toml def test_select_table_returns_nested_table() -> None: """Select nested tables using dotted selectors.""" document = parse_toml('[workspace]\n[workspace.package]\nversion = "0.1.0"\n') - table = bump._select_table(document, ("workspace", "package")) + table = bump_toml.select_table(document, ("workspace", "package")) assert table is document["workspace"]["package"], ( "dotted selector should resolve to the nested [workspace.package] table" ) @@ -19,13 +19,13 @@ def test_select_table_returns_nested_table() -> None: def test_select_table_returns_none_for_missing() -> None: """Selectors that do not resolve to tables return ``None``.""" document = parse_toml("[workspace]\nmembers = []\n") - table = bump._select_table(document, ("workspace", "package")) + table = bump_toml.select_table(document, ("workspace", "package")) assert table is None, "a selector with no matching table should return None" def test_assign_version_handles_absent_table() -> None: - """``_assign_version`` tolerates missing tables.""" - assert bump._assign_version(None, "1.0.0") is False, ( + """``assign_version`` tolerates missing tables.""" + assert bump_toml.assign_version(None, "1.0.0") is False, ( "assigning a version to a missing table should be a no-op" ) @@ -33,7 +33,7 @@ def test_assign_version_handles_absent_table() -> None: def test_assign_version_updates_value() -> None: """Assign a new version when the stored value differs.""" table = parse_toml('[package]\nname = "demo"\nversion = "0.1.0"\n')["package"] - assert bump._assign_version(table, "2.0.0") is True, ( + assert bump_toml.assign_version(table, "2.0.0") is True, ( "assigning a differing version should report a change" ) assert table["version"] == "2.0.0", "the table version should be rewritten" @@ -42,17 +42,17 @@ def test_assign_version_updates_value() -> None: def test_assign_version_detects_existing_value() -> None: """Return ``False`` when the version already matches.""" table = parse_toml('[package]\nversion = "0.1.0"\n')["package"] - assert bump._assign_version(table, "0.1.0") is False, ( + assert bump_toml.assign_version(table, "0.1.0") is False, ( "assigning the current version should report no change" ) def test_value_matches_accepts_plain_strings() -> None: """Strings compare directly when checking for version matches.""" - assert bump._value_matches("1.0.0", "1.0.0") is True, ( + assert bump_toml.value_matches("1.0.0", "1.0.0") is True, ( "equal plain strings should match" ) - assert bump._value_matches("1.0.0", "2.0.0") is False, ( + assert bump_toml.value_matches("1.0.0", "2.0.0") is False, ( "differing plain strings should not match" ) @@ -61,10 +61,10 @@ def test_value_matches_handles_toml_items() -> None: """TOML items compare via their stored string value.""" document = parse_toml('version = "3.0.0"') item = document["version"] - assert bump._value_matches(item, "3.0.0") is True, ( + assert bump_toml.value_matches(item, "3.0.0") is True, ( "a TOML item should match its stored string value" ) - assert bump._value_matches(item, "4.0.0") is False, ( + assert bump_toml.value_matches(item, "4.0.0") is False, ( "a TOML item should not match a different value" ) @@ -80,7 +80,7 @@ def test_select_table_handles_out_of_order_package() -> None: "[package.metadata.docs.rs]\n" "all-features = true\n" ) - table = bump._select_table(document, ("package",)) + table = bump_toml.select_table(document, ("package",)) assert table is not None, "out-of-order [package] table should still be selectable" assert table.get("version") == "0.1.0", ( "selected out-of-order table should expose its version" @@ -96,8 +96,8 @@ def test_assign_version_works_with_out_of_order_table() -> None: "[package.metadata.docs.rs]\n" "all-features = true\n" ) - table = bump._select_table(document, ("package",)) - assert bump._assign_version(table, "2.0.0") is True, ( + table = bump_toml.select_table(document, ("package",)) + assert bump_toml.assign_version(table, "2.0.0") is True, ( "assigning to an out-of-order table should report a change" ) assert table.get("version") == "2.0.0", ( From 2bd555c2f72b47af1654e68fb1911c3cbd438df4 Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 8 Jul 2026 22:34:30 +0200 Subject: [PATCH 4/9] Document the no-compatibility-alias convention (#163) Record in the developers' guide that private symbols carry no stability contract: when a helper moves to a new canonical module, update call sites and test patch targets in the same change instead of leaving aliases or thin wrappers behind. Point recurring churn at an explicit port (as with `CommandRunner`) rather than ad hoc shims. Rewrite the pre-flight validation section, which still described the `publish.py` re-exports and optional-configuration wrapper removed in this branch. --- docs/developers-guide.md | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index fc439913..d30bbf31 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -553,6 +553,21 @@ handle both validation and publish-phase failures through one `except` clause, or catch `PublishError` first when publish-phase failures require distinct handling. + +### Internal APIs carry no compatibility aliases + +Private (underscore-prefixed) functions and modules carry no stability +contract: `lading`'s own application code is the only consumer of its +internals. When a helper moves to a new canonical module, update every call +site and test patch target in the same change — do not leave a module-level +alias or thin wrapper behind to keep old `monkeypatch.setattr` targets +resolving. Tests must patch or invoke the module that defines the helper +(for example, `publish_preflight._run_preflight_checks` rather than a +re-export on `publish`). If an internal seam churns often enough that many +call sites keep breaking, introduce an explicit port (a protocol the call +sites depend on, as with `CommandRunner` in `lading.runtime`) rather than +accreting ad hoc backwards-compatibility shims. + ### Extracted publish modules `publish_plan.py` owns publication planning and plan rendering. Its @@ -874,13 +889,9 @@ reintroduces a second invocation log at any level is pinned by the tests in `lading.commands.publish_preflight` performs workspace validation before any crate is packaged or published. It is the canonical (and only) home of -`_run_preflight_checks` and `_preflight_argument_sets`. `publish.py` -re-exports `_preflight_argument_sets` as a bare module-level alias for -backwards compatibility with existing test patches, and must not re-declare -it. `_run_preflight_checks`, however, is exposed through a thin wrapper in -`publish.py` that preserves the historical optional-`configuration` contract -(resolving configuration via `_ensure_configuration` when the caller omits -it) before delegating to the canonical implementation. The public entry +`_run_preflight_checks` and its helpers. `publish.py` calls the module +directly and holds no aliases or wrappers for its names; tests that patch or +invoke pre-flight helpers must target `publish_preflight` itself. The entry point is: ```python From d8b4e2f9840a05a9ad8759a74eb56edcec2b5ab6 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 14 Jul 2026 03:27:09 +0200 Subject: [PATCH 5/9] Restore public PublishPlanError re-export on publish (#163) The shim sweep removed `PublishPlanError` from `publish.py` alongside the private compatibility aliases. That exception is public API, however: the still-exported `publish.plan_publication()` raises it, so callers catching `except publish.PublishPlanError` began failing with `AttributeError`. Re-export it explicitly with `PublishPlanError as PublishPlanError` so the public entry point and the exception it raises stay catchable from the same module. The no-alias convention in the developers' guide is clarified to scope it to private, underscore-prefixed symbols, and a regression test asserts `publish.PublishPlanError` remains the canonical class and catches planning failures. --- docs/developers-guide.md | 7 +++++++ lading/commands/publish.py | 3 +++ tests/unit/test_publish_planning.py | 26 ++++++++++++++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index d30bbf31..4e7a0b3f 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -568,6 +568,13 @@ call sites keep breaking, introduce an explicit port (a protocol the call sites depend on, as with `CommandRunner` in `lading.runtime`) rather than accreting ad hoc backwards-compatibility shims. +This rule applies to private, underscore-prefixed symbols only. A public +exception type raised by a still-public entry point must remain re-exported +on the module that exposes that entry point: for example, `publish` re-exports +`PublishPlanError as PublishPlanError` so that callers catching +`except publish.PublishPlanError` after a call to `publish.plan_publication()` +continue to work. + ### Extracted publish modules `publish_plan.py` owns publication planning and plan rendering. Its diff --git a/lading/commands/publish.py b/lading/commands/publish.py index 0d253779..7b0aaea2 100644 --- a/lading/commands/publish.py +++ b/lading/commands/publish.py @@ -83,6 +83,9 @@ format_plan, plan_publication, ) +from lading.commands.publish_plan import ( + PublishPlanError as PublishPlanError, # public re-export for plan_publication +) from lading.utils.path import normalise_workspace_root LOGGER = logging.getLogger(__name__) diff --git a/tests/unit/test_publish_planning.py b/tests/unit/test_publish_planning.py index 8e6c9383..2b8fb1a2 100644 --- a/tests/unit/test_publish_planning.py +++ b/tests/unit/test_publish_planning.py @@ -325,6 +325,32 @@ def test_plan_publication_detects_dependency_cycles( assert "dependency cycle" in str(excinfo.value) +def test_publish_reexports_plan_error_for_public_callers( + planning_fixtures: PlanningFixtures, +) -> None: + """``publish.plan_publication`` failures are catchable via ``publish``. + + ``plan_publication`` is public on the ``publish`` module, so the exception + it raises must remain catchable as ``publish.PublishPlanError``. This guards + the public re-export against removal alongside private compatibility shims. + """ + assert publish.PublishPlanError is publish_plan.PublishPlanError + + alpha, beta = _create_cycle( + planning_fixtures, + name_a="alpha", + name_b="beta", + ) + + with pytest.raises(publish.PublishPlanError): + _plan_with_crates( + planning_fixtures.tmp_path, + planning_fixtures.make_workspace, + planning_fixtures.make_config, + (alpha, beta), + ) + + @pytest.mark.parametrize( ("cycle_publish_flags", "excludes", "scenario"), [ From 56ccdbfcda3c6e16b1413477f0f6047e00f0ef85 Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 22:58:27 +0200 Subject: [PATCH 6/9] Document shim inventory and public plan-helper APIs (#163) Address review feedback on the compatibility-shim sweep. Expand the "Internal APIs carry no compatibility aliases" section of the developers' guide with a repository-wide inventory mapping every removed private alias, wrapper, and re-export to its canonical replacement, plus a "Retained boundaries" subsection recording why `publish._invoke`, the public `publish.PublishPlanError` re-export, and the `CommandRunner` port are kept. This gives issue #163 an explicit audit trail. Give the now-public `append_section` and `format_plan` helpers full NumPy-style `Parameters`/`Returns` docstrings, and tighten `test_publish_reexports_plan_error_for_public_callers` with a diagnostic assertion message and a `match="dependency cycle"` constraint on the raised `PublishPlanError`. --- docs/developers-guide.md | 33 ++++++++++++++++++++++++++ lading/commands/publish_plan.py | 36 +++++++++++++++++++++++++++-- tests/unit/test_publish_planning.py | 7 ++++-- 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 4e7a0b3f..4ec4fff5 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -575,6 +575,39 @@ on the module that exposes that entry point: for example, `publish` re-exports `except publish.PublishPlanError` after a call to `publish.plan_publication()` continue to work. +The issue #163 sweep removed the following shims; each row records the +canonical replacement callers and tests now use directly: + +| Removed shim | Location | Canonical replacement | +| --- | --- | --- | +| Eleven `publish_preflight` private aliases (`_preflight_argument_sets`, `_CargoPreflightOptions`, `_apply_compiletest_externs`, `_build_preflight_environment`, `_build_test_arguments`, `_compose_preflight_arguments`, `_normalise_test_excludes`, `_run_aux_build_commands`, `_run_cargo_preflight`, `_validate_lockfile_freshness`, `_verify_clean_working_tree`) | `publish.py` | `lading.commands.publish_preflight` (patch/call the defining module directly) | +| `_run_preflight_checks` thin wrapper | `publish.py` | `publish_preflight._run_preflight_checks` (called directly by `run()`) | +| Re-exports `_append_section`, `_format_plan` | `publish.py` | `publish_plan.append_section`, `publish_plan.format_plan` | +| Re-export `metadata_module` | `publish.py` | `lading.workspace.metadata` | +| Re-export `StripPatchesSetting` | `publish.py` | `lading.config.StripPatchesSetting` | +| Six `bump_toml` re-exports (`_parse_manifest`, `_select_table`, `_assign_version`, `_value_matches`, `_update_dependency_sections`, `_update_dependency_table`) | `bump.py` | `lading.commands.bump_toml` (`parse_manifest`, `select_table`, `assign_version`, `value_matches`, `update_dependency_sections`, `update_dependency_table`) | +| `_log = LOGGER` alias | `bump.py` | the module-level `LOGGER` | +| Private `_append_section` / `_format_plan` | `publish_plan.py` | renamed to public `append_section` / `format_plan` | +| `split_command` / `_split_command` wrapper | `publish_execution.py` | `lading.runtime.subprocess_runner.split_command` | + + +#### Retained boundaries + +Not every module-level indirection is a compatibility shim. The following +boundaries were deliberately kept because they serve a purpose beyond masking +a rename: + +- `publish._invoke` — the dependency-injection seam: it is the default + `CommandRunner` used by `run()`, and tests stub it to intercept subprocess + execution. Not a compatibility alias. +- `publish.PublishPlanError` (re-exported as `PublishPlanError as + PublishPlanError`) — a public exception raised by the still-public + `publish.plan_publication()`; retained so `except publish.PublishPlanError` + keeps working. It is a public-API boundary, not a private shim. +- The `CommandRunner` protocol in `lading.runtime` — the stable port for + execution concerns; the sanctioned alternative to ad hoc shims when a seam + churns. + ### Extracted publish modules `publish_plan.py` owns publication planning and plan rendering. Its diff --git a/lading/commands/publish_plan.py b/lading/commands/publish_plan.py index c91ac273..c007facc 100644 --- a/lading/commands/publish_plan.py +++ b/lading/commands/publish_plan.py @@ -219,14 +219,46 @@ def append_section[T]( header: str, formatter: cabc.Callable[[T], str] = str, ) -> None: - """Append formatted ``items`` to ``lines`` when a section has content.""" + """Append a formatted section to ``lines`` when ``items`` is non-empty. + + Parameters + ---------- + lines: + Accumulator of output lines, mutated in place. When ``items`` is empty + the accumulator is left unchanged. + items: + Entries rendered beneath ``header``. An empty sequence appends nothing. + header: + Section heading emitted before the formatted entries. + formatter: + Callable mapping each item to its display string. Defaults to ``str``. + + Returns + ------- + None + The result is communicated by mutating ``lines`` in place. + """ if items: lines.append(header) lines.extend(f"- {formatter(item)}" for item in items) def format_plan(plan: PublishPlan, *, strip_patches: StripPatchesSetting) -> str: - """Render ``plan`` to a human-readable summary for CLI output.""" + """Render ``plan`` to a human-readable summary for CLI output. + + Parameters + ---------- + plan: + Resolved publication plan whose crate groupings are rendered. + strip_patches: + Strip-patch strategy recorded in the summary header. + + Returns + ------- + str + Multi-line summary listing crates to publish and each skipped group, + suitable for printing to the CLI. + """ lines = [ f"Publish plan for {plan.workspace_root}", f"Strip patch strategy: {strip_patches}", diff --git a/tests/unit/test_publish_planning.py b/tests/unit/test_publish_planning.py index 2b8fb1a2..7acda65b 100644 --- a/tests/unit/test_publish_planning.py +++ b/tests/unit/test_publish_planning.py @@ -334,7 +334,10 @@ def test_publish_reexports_plan_error_for_public_callers( it raises must remain catchable as ``publish.PublishPlanError``. This guards the public re-export against removal alongside private compatibility shims. """ - assert publish.PublishPlanError is publish_plan.PublishPlanError + assert publish.PublishPlanError is publish_plan.PublishPlanError, ( + "publish must re-export the canonical PublishPlanError so callers can " + "catch planning failures via publish.PublishPlanError" + ) alpha, beta = _create_cycle( planning_fixtures, @@ -342,7 +345,7 @@ def test_publish_reexports_plan_error_for_public_callers( name_b="beta", ) - with pytest.raises(publish.PublishPlanError): + with pytest.raises(publish.PublishPlanError, match="dependency cycle"): _plan_with_crates( planning_fixtures.tmp_path, planning_fixtures.make_workspace, From 6b1342d367c26e673775a7afd5fe1399874a38fb Mon Sep 17 00:00:00 2001 From: leynos Date: Sun, 19 Jul 2026 23:00:41 +0200 Subject: [PATCH 7/9] Refresh typos.toml from the Oxford-spelling authority The markdownlint gate runs `scripts/typos_rollout.py`, which regenerates `typos.toml` from its upstream authority. The tracked file had drifted behind the authority, so the gate refreshed it, adding the `polymerize` word family. Commit the deterministic regeneration to keep the working tree clean; the content is generated, not hand-authored, and unrelated to the shim sweep. --- typos.toml | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/typos.toml b/typos.toml index 8c5efed1..3fb9200e 100644 --- a/typos.toml +++ b/typos.toml @@ -1699,6 +1699,24 @@ extend-ignore-re = [ "pluralizers" = "pluralizers" "pluralizes" = "pluralizes" "pluralizing" = "pluralizing" +"polymerisable" = "polymerizable" +"polymerisation" = "polymerization" +"polymerisations" = "polymerizations" +"polymerise" = "polymerize" +"polymerised" = "polymerized" +"polymeriser" = "polymerizer" +"polymerisers" = "polymerizers" +"polymerises" = "polymerizes" +"polymerising" = "polymerizing" +"polymerizable" = "polymerizable" +"polymerization" = "polymerization" +"polymerizations" = "polymerizations" +"polymerize" = "polymerize" +"polymerized" = "polymerized" +"polymerizer" = "polymerizer" +"polymerizers" = "polymerizers" +"polymerizes" = "polymerizes" +"polymerizing" = "polymerizing" "popularisable" = "popularizable" "popularisation" = "popularization" "popularisations" = "popularizations" From e7f740f6ca53c9e307b20cafd6814ede86d54210 Mon Sep 17 00:00:00 2001 From: leynos Date: Tue, 21 Jul 2026 23:12:50 +0200 Subject: [PATCH 8/9] Add doctest examples to publish plan helpers (#163) Address review feedback asking the public `append_section` and `format_plan` helpers to demonstrate representative usage. Add NumPy-style `Examples` sections with executable `>>>` doctests, matching the house style used by helpers such as `command_detail`. The `append_section` example shows both the append and the empty-items no-op; the `format_plan` example renders an empty plan. Both doctests pass under `python -m doctest`. Existing `Parameters` and `Returns` sections are unchanged. --- lading/commands/publish_plan.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/lading/commands/publish_plan.py b/lading/commands/publish_plan.py index c007facc..b408300f 100644 --- a/lading/commands/publish_plan.py +++ b/lading/commands/publish_plan.py @@ -237,6 +237,16 @@ def append_section[T]( ------- None The result is communicated by mutating ``lines`` in place. + + Examples + -------- + >>> lines = ["Header:"] + >>> append_section(lines, ["alpha", "beta"], header="Members:") + >>> lines + ['Header:', 'Members:', '- alpha', '- beta'] + >>> append_section(lines, [], header="Extras:") + >>> lines + ['Header:', 'Members:', '- alpha', '- beta'] """ if items: lines.append(header) @@ -258,6 +268,20 @@ def format_plan(plan: PublishPlan, *, strip_patches: StripPatchesSetting) -> str str Multi-line summary listing crates to publish and each skipped group, suitable for printing to the CLI. + + Examples + -------- + >>> from pathlib import Path + >>> plan = PublishPlan( + ... workspace_root=Path("/ws"), + ... publishable=(), + ... skipped_manifest=(), + ... skipped_configuration=(), + ... ) + >>> print(format_plan(plan, strip_patches="none")) + Publish plan for /ws + Strip patch strategy: none + Crates to publish: none """ lines = [ f"Publish plan for {plan.workspace_root}", From e314b60ceeec011fb09b0d135e45a37973def35b Mon Sep 17 00:00:00 2001 From: leynos Date: Wed, 22 Jul 2026 01:08:52 +0200 Subject: [PATCH 9/9] Caption the shim inventory and make the plan doctest portable (#163) Address two review nits on the compatibility-shim documentation. Add a `#### Shim inventory` subheading before the inventory table in the developers' guide so the table is captioned, consistent with the sibling `#### Retained boundaries` heading. A heading is used rather than bold text because markdownlint MD036 (emphasis-as-heading) is active. Construct the `format_plan` doctest with a relative `Path("ws")` instead of an absolute `Path("/ws")` so the rendered "Publish plan for ws" line is platform-neutral (an absolute path renders differently on Windows). The doctest still passes. --- docs/developers-guide.md | 4 ++-- lading/commands/publish_plan.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/developers-guide.md b/docs/developers-guide.md index 4ec4fff5..a2fa82b4 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -553,7 +553,6 @@ handle both validation and publish-phase failures through one `except` clause, or catch `PublishError` first when publish-phase failures require distinct handling. - ### Internal APIs carry no compatibility aliases Private (underscore-prefixed) functions and modules carry no stability @@ -575,6 +574,8 @@ on the module that exposes that entry point: for example, `publish` re-exports `except publish.PublishPlanError` after a call to `publish.plan_publication()` continue to work. +#### Shim inventory + The issue #163 sweep removed the following shims; each row records the canonical replacement callers and tests now use directly: @@ -590,7 +591,6 @@ canonical replacement callers and tests now use directly: | Private `_append_section` / `_format_plan` | `publish_plan.py` | renamed to public `append_section` / `format_plan` | | `split_command` / `_split_command` wrapper | `publish_execution.py` | `lading.runtime.subprocess_runner.split_command` | - #### Retained boundaries Not every module-level indirection is a compatibility shim. The following diff --git a/lading/commands/publish_plan.py b/lading/commands/publish_plan.py index b408300f..a7bcc603 100644 --- a/lading/commands/publish_plan.py +++ b/lading/commands/publish_plan.py @@ -273,13 +273,13 @@ def format_plan(plan: PublishPlan, *, strip_patches: StripPatchesSetting) -> str -------- >>> from pathlib import Path >>> plan = PublishPlan( - ... workspace_root=Path("/ws"), + ... workspace_root=Path("ws"), ... publishable=(), ... skipped_manifest=(), ... skipped_configuration=(), ... ) >>> print(format_plan(plan, strip_patches="none")) - Publish plan for /ws + Publish plan for ws Strip patch strategy: none Crates to publish: none """