refactor(cli): split out business logic into separate class

Allows easier testing.

Author: finswimmer

src/poetry/console/commands/build.py

@@ -1,9 +1,12 @@
 from __future__ import annotations
 
+import dataclasses
+
 from pathlib import Path
 from typing import TYPE_CHECKING
 from typing import Any
 from typing import ClassVar
+from typing import Literal
 
 from cleo.helpers import option
 from poetry.core.constraints.version import Version
@@ -18,83 +21,33 @@
 if TYPE_CHECKING:
     from collections.abc import Callable
 
-    from build import DistributionType
     from cleo.io.inputs.option import Option
+    from cleo.io.io import IO
 
+    from poetry.poetry import Poetry
+    from poetry.utils.env import Env
 
-class BuildCommand(EnvCommand):
-    name = "build"
-    description = "Builds a package, as a tarball and a wheel by default."
+DistributionType = Literal["sdist", "wheel"]
 
-    options: ClassVar[list[Option]] = [
-        option("format", "f", "Limit the format to either sdist or wheel.", flag=False),
-        option(
-            "clean",
-            description="Clean output directory before building.",
-            flag=True,
-        ),
-        option(
-            "local-version",
-            "l",
-            "Add or replace a local version label to the build. (<warning>Deprecated</warning>)",
-            flag=False,
-        ),
-        option(
-            "output",
-            "o",
-            "Set output directory for build artifacts. Default is `dist`.",
-            default="dist",
-            flag=False,
-        ),
-        option(
-            "config-settings",
-            description="Provide config settings that should be passed to backend in <key>=<value> format.",
-            flag=False,
-            multiple=True,
-        ),
-    ]
 
-    loggers: ClassVar[list[str]] = [
-        "poetry.core.masonry.builders.builder",
-        "poetry.core.masonry.builders.sdist",
-        "poetry.core.masonry.builders.wheel",
-    ]
+@dataclasses.dataclass(frozen=True)
+class BuildOptions:
+    clean: bool
+    formats: list[DistributionType]
+    output: str
+    config_settings: dict[str, Any] = dataclasses.field(default_factory=dict)
 
-    def _requested_formats(self) -> list[DistributionType]:
-        fmt = self.option("format") or "all"
-        formats: list[DistributionType]
+    def __post_init__(self) -> None:
+        for fmt in self.formats:
+            if fmt not in BUILD_FORMATS:
+                raise ValueError(f"Invalid format: {fmt}")
 
-        if fmt in BUILD_FORMATS:
-            formats = [fmt]  # type: ignore[list-item]
-        elif fmt == "all":
-            formats = list(BUILD_FORMATS.keys())  # type: ignore[arg-type]
-        else:
-            raise ValueError(f"Invalid format: {fmt}")
 
-        return formats
-
-    def _config_settings(self) -> dict[str, str]:
-        config_settings = {}
-
-        if local_version_label := self.option("local-version"):
-            self.line_error(
-                f"<warning>`<fg=yellow;options=bold>--local-version</>` is deprecated."
-                f" Use `<fg=yellow;options=bold>--config-settings local-version={local_version_label}</>`"
-                f" instead.</warning>"
-            )
-            config_settings["local-version"] = local_version_label
-
-        for config_setting in self.option("config-settings"):
-            if "=" not in config_setting:
-                raise ValueError(
-                    f"Invalid config setting format: {config_setting}. "
-                    "Config settings must be in the format 'key=value'"
-                )
-
-            key, _, value = config_setting.partition("=")
-            config_settings[key] = value
-
-        return config_settings
+class BuildHandler:
+    def __init__(self, poetry: Poetry, env: Env, io: IO) -> None:
+        self.poetry = poetry
+        self.env = env
+        self.io = io
 
     def _build(
         self,
@@ -103,9 +56,6 @@ def _build(
         target_dir: Path,
         config_settings: dict[str, Any],
     ) -> None:
-        if fmt not in BUILD_FORMATS:
-            raise ValueError(f"Invalid format: {fmt}")
-
         builder = BUILD_FORMATS[fmt]
 
         builder(
@@ -121,9 +71,6 @@ def _isolated_build(
         target_dir: Path,
         config_settings: dict[str, Any],
     ) -> None:
-        if fmt not in BUILD_FORMATS:
-            raise ValueError(f"Invalid format: {fmt}")
-
         with isolated_builder(
             source=self.poetry.file.path.parent,
             distribution=fmt,
@@ -167,32 +114,133 @@ def _get_builder(self) -> Callable[..., None]:
 
         return self._build
 
-    def handle(self) -> int:
+    def build(self, options: BuildOptions) -> int:
         if not self.poetry.is_package_mode:
-            self.line_error("Building a package is not possible in non-package mode.")
+            self.io.write_error_line(
+                "Building a package is not possible in non-package mode."
+            )
             return 1
 
-        dist_dir = Path(self.option("output"))
+        dist_dir = Path(options.output)
         package = self.poetry.package
-        self.line(
+        self.io.write_line(
             f"Building <c1>{package.pretty_name}</c1> (<c2>{package.version}</c2>)"
         )
 
         if not dist_dir.is_absolute():
             dist_dir = self.poetry.pyproject_path.parent / dist_dir
 
-        if self.option("clean"):
+        if options.clean:
             remove_directory(path=dist_dir, force=True)
 
         build = self._get_builder()
 
-        for fmt in self._requested_formats():
-            self.line(f"Building <info>{fmt}</info>")
+        for fmt in options.formats:
+            self.io.write_line(f"Building <info>{fmt}</info>")
             build(
                 fmt,
                 executable=self.env.python,
                 target_dir=dist_dir,
-                config_settings=self._config_settings(),
+                config_settings=options.config_settings,
             )
 
         return 0
+
+
+class BuildCommand(EnvCommand):
+    name = "build"
+    description = "Builds a package, as a tarball and a wheel by default."
+
+    options: ClassVar[list[Option]] = [
+        option("format", "f", "Limit the format to either sdist or wheel.", flag=False),
+        option(
+            "clean",
+            description="Clean output directory before building.",
+            flag=True,
+        ),
+        option(
+            "local-version",
+            "l",
+            "Add or replace a local version label to the build. (<warning>Deprecated</warning>)",
+            flag=False,
+        ),
+        option(
+            "output",
+            "o",
+            "Set output directory for build artifacts. Default is `dist`.",
+            default="dist",
+            flag=False,
+        ),
+        option(
+            "config-settings",
+            description="Provide config settings that should be passed to backend in <key>=<value> format.",
+            flag=False,
+            multiple=True,
+        ),
+    ]
+
+    loggers: ClassVar[list[str]] = [
+        "poetry.core.masonry.builders.builder",
+        "poetry.core.masonry.builders.sdist",
+        "poetry.core.masonry.builders.wheel",
+    ]
+
+    @staticmethod
+    def _prepare_config_settings(
+        local_version: str | None, config_settings: list[str] | None, io: IO
+    ) -> dict[str, str]:
+        config_settings = config_settings or []
+        result = {}
+
+        if local_version:
+            io.write_error_line(
+                f"<warning>`<fg=yellow;options=bold>--local-version</>` is deprecated."
+                f" Use `<fg=yellow;options=bold>--config-settings local-version={local_version}</>`"
+                f" instead.</warning>"
+            )
+            result["local-version"] = local_version
+
+        for config_setting in config_settings:
+            if "=" not in config_setting:
+                raise ValueError(
+                    f"Invalid config setting format: {config_setting}. "
+                    "Config settings must be in the format 'key=value'"
+                )
+
+            key, _, value = config_setting.partition("=")
+            result[key] = value
+
+        return result
+
+    @staticmethod
+    def _prepare_formats(fmt: str | None) -> list[DistributionType]:
+        fmt = fmt or "all"
+        formats: list[DistributionType]
+
+        if fmt in BUILD_FORMATS:
+            formats = [fmt]  # type: ignore[list-item]
+        elif fmt == "all":
+            formats = list(BUILD_FORMATS.keys())  # type: ignore[arg-type]
+        else:
+            raise ValueError(f"Invalid format: {fmt}")
+
+        return formats
+
+    def handle(self) -> int:
+        build_handler = BuildHandler(
+            poetry=self.poetry,
+            env=self.env,
+            io=self.io,
+        )
+        build_options = BuildOptions(
+            clean=self.option("clean"),
+            formats=self._prepare_formats(self.option("format")),
+            output=self.option("output"),
+            config_settings=self._prepare_config_settings(
+                local_version=self.option("local-version"),
+                config_settings=self.option("config-settings"),
+                io=self.io,
+            ),
+        )
+
+        return build_handler.build(options=build_options)

tests/console/commands/test_build.py

@@ -7,9 +7,13 @@
 
 import pytest
 
+from cleo.io.null_io import NullIO
 from cleo.testers.application_tester import ApplicationTester
 
 from poetry.console.application import Application
+from poetry.console.commands.build import BuildCommand
+from poetry.console.commands.build import BuildHandler
+from poetry.console.commands.build import BuildOptions
 from poetry.factory import Factory
 from poetry.utils.helpers import remove_directory
 from tests.helpers import with_working_directory
@@ -19,6 +23,7 @@
     from pathlib import Path
 
     from cleo.testers.command_tester import CommandTester
+    from pytest_mock import MockerFixture
 
     from poetry.poetry import Poetry
     from poetry.utils.env import VirtualEnv
@@ -211,3 +216,83 @@ def test_build_relative_directory_src_layout(
         build_dir = tmp_project_path / "dist"
 
         assert len(list(build_dir.iterdir())) == 2
+
+
+def test_build_options_validate_formats() -> None:
+    with pytest.raises(ValueError, match="Invalid format: UNKNOWN"):
+        _ = BuildOptions(clean=True, formats=["sdist", "UNKNOWN"], output="dist")  # type: ignore[list-item]
+
+
+def test_prepare_config_settings() -> None:
+    config_settings = BuildCommand._prepare_config_settings(
+        local_version="42",
+        config_settings=["setting_1=value_1", "setting_2=value_2"],
+        io=NullIO(),
+    )
+
+    assert config_settings == {
+        "local-version": "42",
+        "setting_1": "value_1",
+        "setting_2": "value_2",
+    }
+
+
+@pytest.mark.parametrize(
+    ["fmt", "expected_formats"],
+    [
+        ("all", ["sdist", "wheel"]),
+        (None, ["sdist", "wheel"]),
+        ("sdist", ["sdist"]),
+        ("wheel", ["wheel"]),
+    ],
+)
+def test_prepare_formats(fmt: str | None, expected_formats: list[str]) -> None:
+    formats = BuildCommand._prepare_formats(fmt)
+    assert formats == expected_formats
+
+
+def test_prepare_formats_raise_on_invalid() -> None:
+    with pytest.raises(ValueError, match="Invalid format: UNKNOWN"):
+        _ = BuildCommand._prepare_formats("UNKNOWN")
+
+
+@pytest.mark.parametrize(
+    ["project", "isolated_build"],
+    [
+        ("core_in_range", False),
+        ("core_not_in_range", True),
+        ("has_build_script", True),
+        ("multiple_build_deps", True),
+        ("no_core", True),
+        ("core_from_git", True),
+    ],
+)
+def test_requires_isolated_build(
+    project: str,
+    isolated_build: bool,
+    fixture_dir: FixtureDirGetter,
+    mocker: MockerFixture,
+) -> None:
+    poetry = Factory().create_poetry(fixture_dir(f"build_systems/{project}"))
+    handler = BuildHandler(poetry=poetry, env=mocker.Mock(), io=NullIO())
+
+    assert handler._requires_isolated_build() is isolated_build
+
+
+def test_build_handler_build_isolated(
+    fixture_dir: FixtureDirGetter, mocker: MockerFixture
+) -> None:
+    from build import ProjectBuilder
+
+    poetry = Factory().create_poetry(fixture_dir("build_systems/has_build_script"))
+
+    mock_builder = mocker.MagicMock(spec=ProjectBuilder)
+    mock_isolated_builder = mocker.patch(
+        "poetry.console.commands.build.isolated_builder"
+    )
+    mock_isolated_builder.return_value.__enter__.return_value = mock_builder
+
+    handler = BuildHandler(poetry=poetry, env=mocker.Mock(), io=NullIO())
+    handler.build(BuildOptions(clean=True, formats=["wheel"], output="dist"))
+
+    assert mock_builder.build.call_count == 1

tests/fixtures/build_systems/core_from_git/README.md

@@ -0,0 +1,2 @@
+My Package
+==========