From 880c70bd58bfb4d5d43be66e82f6c70b350e8a31 Mon Sep 17 00:00:00 2001 From: Evgeniy Alekseev Date: Wed, 31 Mar 2021 04:15:55 +0300 Subject: [PATCH] constistent classmethod and staticmethod usage General idea is to use classmethod for every constructor and statismethod otherwise. Also use self and cls whenever it's possible to call static and class methods --- src/ahriman/application/lock.py | 18 +++--- src/ahriman/core/build_tools/task.py | 2 +- src/ahriman/core/report/report.py | 34 +++++----- src/ahriman/core/repository/executor.py | 6 +- src/ahriman/core/status/client.py | 31 +++++----- src/ahriman/core/upload/upload.py | 29 +++++---- src/ahriman/models/build_status.py | 2 +- src/ahriman/models/package.py | 44 +++++++------ src/ahriman/models/package_description.py | 2 +- src/ahriman/models/report_settings.py | 7 ++- src/ahriman/models/sign_settings.py | 9 +-- src/ahriman/models/upload_settings.py | 9 +-- tests/ahriman/application/test_lock.py | 62 +++++++++---------- tests/ahriman/core/report/test_report.py | 4 +- tests/ahriman/core/status/test_client.py | 32 +++++----- tests/ahriman/core/test_configuration.py | 4 +- .../{test_uploader.py => test_upload.py} | 6 +- tests/ahriman/models/test_package.py | 36 +++++------ 18 files changed, 176 insertions(+), 161 deletions(-) rename tests/ahriman/core/upload/{test_uploader.py => test_upload.py} (79%) diff --git a/src/ahriman/application/lock.py b/src/ahriman/application/lock.py index ea0dbe6d..1cede7f4 100644 --- a/src/ahriman/application/lock.py +++ b/src/ahriman/application/lock.py @@ -80,7 +80,7 @@ class Lock: :param exc_tb: exception traceback if any :return: always False (do not suppress any exception) """ - self.remove() + self.clear() status = BuildStatusEnum.Success if exc_val is None else BuildStatusEnum.Failed self.reporter.update_self(status) return False @@ -96,6 +96,14 @@ class Lock: if current_uid != root_uid: raise UnsafeRun(current_uid, root_uid) + def clear(self) -> None: + """ + remove lock file + """ + if self.path is None: + return + self.path.unlink(missing_ok=True) + def create(self) -> None: """ create lock file @@ -106,11 +114,3 @@ class Lock: self.path.touch(exist_ok=self.force) except FileExistsError: raise DuplicateRun() - - def remove(self) -> None: - """ - remove lock file - """ - if self.path is None: - return - self.path.unlink(missing_ok=True) diff --git a/src/ahriman/core/build_tools/task.py b/src/ahriman/core/build_tools/task.py index a64d74ff..93ad46b1 100644 --- a/src/ahriman/core/build_tools/task.py +++ b/src/ahriman/core/build_tools/task.py @@ -123,4 +123,4 @@ class Task: if self.cache_path.is_dir(): # no need to clone whole repository, just copy from cache first shutil.copytree(self.cache_path, git_path) - return Task.fetch(git_path, self.package.git_url) + return self.fetch(git_path, self.package.git_url) diff --git a/src/ahriman/core/report/report.py b/src/ahriman/core/report/report.py index 67b6b8b4..20406ae9 100644 --- a/src/ahriman/core/report/report.py +++ b/src/ahriman/core/report/report.py @@ -17,9 +17,11 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # +from __future__ import annotations + import logging -from typing import Iterable +from typing import Iterable, Type from ahriman.core.configuration import Configuration from ahriman.core.exceptions import ReportFailed @@ -45,30 +47,34 @@ class Report: self.architecture = architecture self.configuration = configuration - @staticmethod - def run(architecture: str, configuration: Configuration, target: str, packages: Iterable[Package]) -> None: + @classmethod + def load(cls: Type[Report], architecture: str, configuration: Configuration, target: str) -> Report: """ - run report generation + load client from settings :param architecture: repository architecture :param configuration: configuration instance :param target: target to generate report (e.g. html) - :param packages: list of packages to generate report + :return: client according to current settings """ provider = ReportSettings.from_option(target) if provider == ReportSettings.HTML: from ahriman.core.report.html import HTML - report: Report = HTML(architecture, configuration) - else: - report = Report(architecture, configuration) - - try: - report.generate(packages) - except Exception: - report.logger.exception(f"report generation failed for target {provider.name}") - raise ReportFailed() + return HTML(architecture, configuration) + return cls(architecture, configuration) # should never happen def generate(self, packages: Iterable[Package]) -> None: """ generate report for the specified packages :param packages: list of packages to generate report """ + + def run(self, packages: Iterable[Package]) -> None: + """ + run report generation + :param packages: list of packages to generate report + """ + try: + self.generate(packages) + except Exception: + self.logger.exception("report generation failed") + raise ReportFailed() diff --git a/src/ahriman/core/repository/executor.py b/src/ahriman/core/repository/executor.py index 5710ef55..2a94eb0f 100644 --- a/src/ahriman/core/repository/executor.py +++ b/src/ahriman/core/repository/executor.py @@ -108,7 +108,8 @@ class Executor(Cleaner): if targets is None: targets = self.configuration.getlist("report", "target") for target in targets: - Report.run(self.architecture, self.configuration, target, self.packages()) + runner = Report.load(self.architecture, self.configuration, target) + runner.run(self.packages()) def process_sync(self, targets: Optional[Iterable[str]]) -> None: """ @@ -118,7 +119,8 @@ class Executor(Cleaner): if targets is None: targets = self.configuration.getlist("upload", "target") for target in targets: - Upload.run(self.architecture, self.configuration, target, self.paths.repository) + runner = Upload.load(self.architecture, self.configuration, target) + runner.run(self.paths.repository) def process_update(self, packages: Iterable[Path]) -> Path: """ diff --git a/src/ahriman/core/status/client.py b/src/ahriman/core/status/client.py index 937d0a6f..d7bbfb79 100644 --- a/src/ahriman/core/status/client.py +++ b/src/ahriman/core/status/client.py @@ -19,7 +19,7 @@ # from __future__ import annotations -from typing import List, Optional, Tuple +from typing import List, Optional, Tuple, Type from ahriman.core.configuration import Configuration from ahriman.models.build_status import BuildStatus, BuildStatusEnum @@ -31,6 +31,20 @@ class Client: base build status reporter client """ + @classmethod + def load(cls: Type[Client], configuration: Configuration) -> Client: + """ + load client from settings + :param configuration: configuration instance + :return: client according to current settings + """ + host = configuration.get("web", "host", fallback=None) + port = configuration.getint("web", "port", fallback=None) + if host is not None and port is not None: + from ahriman.core.status.web_client import WebClient + return WebClient(host, port) + return cls() + def add(self, package: Package, status: BuildStatusEnum) -> None: """ add new package with status @@ -109,18 +123,3 @@ class Client: :param package: current package properties """ return self.add(package, BuildStatusEnum.Unknown) - - @staticmethod - def load(configuration: Configuration) -> Client: - """ - load client from settings - :param configuration: configuration instance - :return: client according to current settings - """ - host = configuration.get("web", "host", fallback=None) - port = configuration.getint("web", "port", fallback=None) - if host is None or port is None: - return Client() - - from ahriman.core.status.web_client import WebClient - return WebClient(host, port) diff --git a/src/ahriman/core/upload/upload.py b/src/ahriman/core/upload/upload.py index 93de8626..c5016404 100644 --- a/src/ahriman/core/upload/upload.py +++ b/src/ahriman/core/upload/upload.py @@ -17,9 +17,12 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . # +from __future__ import annotations + import logging from pathlib import Path +from typing import Type from ahriman.core.configuration import Configuration from ahriman.core.exceptions import SyncFailed @@ -44,29 +47,33 @@ class Upload: self.architecture = architecture self.config = configuration - @staticmethod - def run(architecture: str, configuration: Configuration, target: str, path: Path) -> None: + @classmethod + def load(cls: Type[Upload], architecture: str, configuration: Configuration, target: str) -> Upload: """ - run remote sync + load client from settings :param architecture: repository architecture :param configuration: configuration instance :param target: target to run sync (e.g. s3) - :param path: local path to sync + :return: client according to current settings """ provider = UploadSettings.from_option(target) if provider == UploadSettings.Rsync: from ahriman.core.upload.rsync import Rsync - upload: Upload = Rsync(architecture, configuration) - elif provider == UploadSettings.S3: + return Rsync(architecture, configuration) + if provider == UploadSettings.S3: from ahriman.core.upload.s3 import S3 - upload = S3(architecture, configuration) - else: - upload = Upload(architecture, configuration) + return S3(architecture, configuration) + return cls(architecture, configuration) # should never happen + def run(self, path: Path) -> None: + """ + run remote sync + :param path: local path to sync + """ try: - upload.sync(path) + self.sync(path) except Exception: - upload.logger.exception(f"remote sync failed for {provider.name}") + self.logger.exception("remote sync failed") raise SyncFailed() def sync(self, path: Path) -> None: diff --git a/src/ahriman/models/build_status.py b/src/ahriman/models/build_status.py index 7489ff86..2dcbca1a 100644 --- a/src/ahriman/models/build_status.py +++ b/src/ahriman/models/build_status.py @@ -63,7 +63,7 @@ class BuildStatus: """ build status holder :ivar status: build status - :ivar _timestamp: build status update time + :ivar timestamp: build status update time """ def __init__(self, status: Union[BuildStatusEnum, str, None] = None, diff --git a/src/ahriman/models/package.py b/src/ahriman/models/package.py index d2a05fbd..17616284 100644 --- a/src/ahriman/models/package.py +++ b/src/ahriman/models/package.py @@ -156,6 +156,27 @@ class Package: aur_url=dump["aur_url"], packages=packages) + @classmethod + def load(cls: Type[Package], path: Union[Path, str], pacman: Pacman, aur_url: str) -> Package: + """ + package constructor from available sources + :param path: one of path to sources directory, path to archive or package name/base + :param pacman: alpm wrapper instance (required to load from archive) + :param aur_url: AUR root url + :return: package properties + """ + try: + maybe_path = Path(path) + if maybe_path.is_dir(): + return cls.from_build(maybe_path, aur_url) + if maybe_path.is_file(): + return cls.from_archive(maybe_path, pacman, aur_url) + return cls.from_aur(str(path), aur_url) + except InvalidPackageInfo: + raise + except Exception as e: + raise InvalidPackageInfo(str(e)) + @staticmethod def dependencies(path: Path) -> Set[str]: """ @@ -194,29 +215,6 @@ class Package: prefix = f"{epoch}:" if epoch else "" return f"{prefix}{pkgver}-{pkgrel}" - @staticmethod - def load(path: Union[Path, str], pacman: Pacman, aur_url: str) -> Package: - """ - package constructor from available sources - :param path: one of path to sources directory, path to archive or package name/base - :param pacman: alpm wrapper instance (required to load from archive) - :param aur_url: AUR root url - :return: package properties - """ - try: - maybe_path = Path(path) - if maybe_path.is_dir(): - package: Package = Package.from_build(maybe_path, aur_url) - elif maybe_path.is_file(): - package = Package.from_archive(maybe_path, pacman, aur_url) - else: - package = Package.from_aur(str(path), aur_url) - return package - except InvalidPackageInfo: - raise - except Exception as e: - raise InvalidPackageInfo(str(e)) - def actual_version(self, paths: RepositoryPaths) -> str: """ additional method to handle VCS package versions diff --git a/src/ahriman/models/package_description.py b/src/ahriman/models/package_description.py index e71a2e50..1a5f70d8 100644 --- a/src/ahriman/models/package_description.py +++ b/src/ahriman/models/package_description.py @@ -65,7 +65,7 @@ class PackageDescription: :param path: path to package archive :return: package properties based on tarball """ - return PackageDescription( + return cls( architecture=package.arch, archive_size=package.size, build_date=package.builddate, diff --git a/src/ahriman/models/report_settings.py b/src/ahriman/models/report_settings.py index f13bee33..d850e268 100644 --- a/src/ahriman/models/report_settings.py +++ b/src/ahriman/models/report_settings.py @@ -20,6 +20,7 @@ from __future__ import annotations from enum import Enum, auto +from typing import Type from ahriman.core.exceptions import InvalidOption @@ -32,13 +33,13 @@ class ReportSettings(Enum): HTML = auto() - @staticmethod - def from_option(value: str) -> ReportSettings: + @classmethod + def from_option(cls: Type[ReportSettings], value: str) -> ReportSettings: """ construct value from configuration :param value: configuration value :return: parsed value """ if value.lower() in ("html",): - return ReportSettings.HTML + return cls.HTML raise InvalidOption(value) diff --git a/src/ahriman/models/sign_settings.py b/src/ahriman/models/sign_settings.py index 2919f720..918c38a2 100644 --- a/src/ahriman/models/sign_settings.py +++ b/src/ahriman/models/sign_settings.py @@ -20,6 +20,7 @@ from __future__ import annotations from enum import Enum, auto +from typing import Type from ahriman.core.exceptions import InvalidOption @@ -34,15 +35,15 @@ class SignSettings(Enum): SignPackages = auto() SignRepository = auto() - @staticmethod - def from_option(value: str) -> SignSettings: + @classmethod + def from_option(cls: Type[SignSettings], value: str) -> SignSettings: """ construct value from configuration :param value: configuration value :return: parsed value """ if value.lower() in ("package", "packages", "sign-package"): - return SignSettings.SignPackages + return cls.SignPackages if value.lower() in ("repository", "sign-repository"): - return SignSettings.SignRepository + return cls.SignRepository raise InvalidOption(value) diff --git a/src/ahriman/models/upload_settings.py b/src/ahriman/models/upload_settings.py index a892353a..1fec402c 100644 --- a/src/ahriman/models/upload_settings.py +++ b/src/ahriman/models/upload_settings.py @@ -20,6 +20,7 @@ from __future__ import annotations from enum import Enum, auto +from typing import Type from ahriman.core.exceptions import InvalidOption @@ -34,15 +35,15 @@ class UploadSettings(Enum): Rsync = auto() S3 = auto() - @staticmethod - def from_option(value: str) -> UploadSettings: + @classmethod + def from_option(cls: Type[UploadSettings], value: str) -> UploadSettings: """ construct value from configuration :param value: configuration value :return: parsed value """ if value.lower() in ("rsync",): - return UploadSettings.Rsync + return cls.Rsync if value.lower() in ("s3",): - return UploadSettings.S3 + return cls.S3 raise InvalidOption(value) diff --git a/tests/ahriman/application/test_lock.py b/tests/ahriman/application/test_lock.py index c3573558..58cf1956 100644 --- a/tests/ahriman/application/test_lock.py +++ b/tests/ahriman/application/test_lock.py @@ -15,14 +15,14 @@ def test_enter(lock: Lock, mocker: MockerFixture) -> None: must process with context manager """ check_user_mock = mocker.patch("ahriman.application.lock.Lock.check_user") - remove_mock = mocker.patch("ahriman.application.lock.Lock.remove") + clear_mock = mocker.patch("ahriman.application.lock.Lock.clear") create_mock = mocker.patch("ahriman.application.lock.Lock.create") update_status_mock = mocker.patch("ahriman.core.status.client.Client.update_self") with lock: pass check_user_mock.assert_called_once() - remove_mock.assert_called_once() + clear_mock.assert_called_once() create_mock.assert_called_once() update_status_mock.assert_has_calls([ mock.call(BuildStatusEnum.Building), @@ -35,7 +35,7 @@ def test_exit_with_exception(lock: Lock, mocker: MockerFixture) -> None: must process with context manager in case if exception raised """ mocker.patch("ahriman.application.lock.Lock.check_user") - mocker.patch("ahriman.application.lock.Lock.remove") + mocker.patch("ahriman.application.lock.Lock.clear") mocker.patch("ahriman.application.lock.Lock.create") update_status_mock = mocker.patch("ahriman.core.status.client.Client.update_self") @@ -79,6 +79,34 @@ def test_check_user_unsafe(lock: Lock) -> None: lock.check_user() +def test_clear(lock: Lock) -> None: + """ + must remove lock file + """ + lock.path = Path(tempfile.mktemp()) + lock.path.touch() + + lock.clear() + assert not lock.path.is_file() + + +def test_clear_missing(lock: Lock) -> None: + """ + must not fail on lock removal if file is missing + """ + lock.path = Path(tempfile.mktemp()) + lock.clear() + + +def test_clear_skip(lock: Lock, mocker: MockerFixture) -> None: + """ + must skip removal if no file set + """ + unlink_mock = mocker.patch("pathlib.Path.unlink") + lock.clear() + unlink_mock.assert_not_called() + + def test_create(lock: Lock) -> None: """ must create lock @@ -121,31 +149,3 @@ def test_create_unsafe(lock: Lock) -> None: lock.create() lock.path.unlink() - - -def test_remove(lock: Lock) -> None: - """ - must remove lock file - """ - lock.path = Path(tempfile.mktemp()) - lock.path.touch() - - lock.remove() - assert not lock.path.is_file() - - -def test_remove_missing(lock: Lock) -> None: - """ - must not fail on lock removal if file is missing - """ - lock.path = Path(tempfile.mktemp()) - lock.remove() - - -def test_remove_skip(lock: Lock, mocker: MockerFixture) -> None: - """ - must skip removal if no file set - """ - unlink_mock = mocker.patch("pathlib.Path.unlink") - lock.remove() - unlink_mock.assert_not_called() diff --git a/tests/ahriman/core/report/test_report.py b/tests/ahriman/core/report/test_report.py index c53bf649..2d1acbb8 100644 --- a/tests/ahriman/core/report/test_report.py +++ b/tests/ahriman/core/report/test_report.py @@ -15,7 +15,7 @@ def test_report_failure(configuration: Configuration, mocker: MockerFixture) -> """ mocker.patch("ahriman.core.report.html.HTML.generate", side_effect=Exception()) with pytest.raises(ReportFailed): - Report.run("x86_64", configuration, ReportSettings.HTML.name, Path("path")) + Report.load("x86_64", configuration, ReportSettings.HTML.name).run(Path("path")) def test_report_html(configuration: Configuration, mocker: MockerFixture) -> None: @@ -23,5 +23,5 @@ def test_report_html(configuration: Configuration, mocker: MockerFixture) -> Non must generate html report """ report_mock = mocker.patch("ahriman.core.report.html.HTML.generate") - Report.run("x86_64", configuration, ReportSettings.HTML.name, Path("path")) + Report.load("x86_64", configuration, ReportSettings.HTML.name).run(Path("path")) report_mock.assert_called_once() diff --git a/tests/ahriman/core/status/test_client.py b/tests/ahriman/core/status/test_client.py index 3a74b5c7..8b1c4e0a 100644 --- a/tests/ahriman/core/status/test_client.py +++ b/tests/ahriman/core/status/test_client.py @@ -7,6 +7,22 @@ from ahriman.models.build_status import BuildStatusEnum from ahriman.models.package import Package +def test_load_dummy_client(configuration: Configuration) -> None: + """ + must load dummy client if no settings set + """ + assert isinstance(Client.load(configuration), Client) + + +def test_load_full_client(configuration: Configuration) -> None: + """ + must load full client if no settings set + """ + configuration.set("web", "host", "localhost") + configuration.set("web", "port", "8080") + assert isinstance(Client.load(configuration), WebClient) + + def test_add(client: Client, package_ahriman: Package) -> None: """ must process package addition without errors @@ -98,19 +114,3 @@ def test_set_unknown(client: Client, package_ahriman: Package, mocker: MockerFix client.set_unknown(package_ahriman) add_mock.assert_called_with(package_ahriman, BuildStatusEnum.Unknown) - - -def test_load_dummy_client(configuration: Configuration) -> None: - """ - must load dummy client if no settings set - """ - assert isinstance(Client.load(configuration), Client) - - -def test_load_full_client(configuration: Configuration) -> None: - """ - must load full client if no settings set - """ - configuration.set("web", "host", "localhost") - configuration.set("web", "port", "8080") - assert isinstance(Client.load(configuration), WebClient) diff --git a/tests/ahriman/core/test_configuration.py b/tests/ahriman/core/test_configuration.py index 36fadbe7..198dae0d 100644 --- a/tests/ahriman/core/test_configuration.py +++ b/tests/ahriman/core/test_configuration.py @@ -14,8 +14,8 @@ def test_from_path(mocker: MockerFixture) -> None: load_logging_mock = mocker.patch("ahriman.core.configuration.Configuration.load_logging") path = Path("path") - config = Configuration.from_path(path, "x86_64", True) - assert config.path == path + configuration = Configuration.from_path(path, "x86_64", True) + assert configuration.path == path read_mock.assert_called_with(path) load_includes_mock.assert_called_once() load_logging_mock.assert_called_once() diff --git a/tests/ahriman/core/upload/test_uploader.py b/tests/ahriman/core/upload/test_upload.py similarity index 79% rename from tests/ahriman/core/upload/test_uploader.py rename to tests/ahriman/core/upload/test_upload.py index 1e1c5140..99815c57 100644 --- a/tests/ahriman/core/upload/test_uploader.py +++ b/tests/ahriman/core/upload/test_upload.py @@ -15,7 +15,7 @@ def test_upload_failure(configuration: Configuration, mocker: MockerFixture) -> """ mocker.patch("ahriman.core.upload.rsync.Rsync.sync", side_effect=Exception()) with pytest.raises(SyncFailed): - Upload.run("x86_64", configuration, UploadSettings.Rsync.name, Path("path")) + Upload.load("x86_64", configuration, UploadSettings.Rsync.name).run(Path("path")) def test_upload_rsync(configuration: Configuration, mocker: MockerFixture) -> None: @@ -23,7 +23,7 @@ def test_upload_rsync(configuration: Configuration, mocker: MockerFixture) -> No must upload via rsync """ upload_mock = mocker.patch("ahriman.core.upload.rsync.Rsync.sync") - Upload.run("x86_64", configuration, UploadSettings.Rsync.name, Path("path")) + Upload.load("x86_64", configuration, UploadSettings.Rsync.name).run(Path("path")) upload_mock.assert_called_once() @@ -32,5 +32,5 @@ def test_upload_s3(configuration: Configuration, mocker: MockerFixture) -> None: must upload via s3 """ upload_mock = mocker.patch("ahriman.core.upload.s3.S3.sync") - Upload.run("x86_64", configuration, UploadSettings.S3.name, Path("path")) + Upload.load("x86_64", configuration, UploadSettings.S3.name).run(Path("path")) upload_mock.assert_called_once() diff --git a/tests/ahriman/models/test_package.py b/tests/ahriman/models/test_package.py index e6847232..a547a8bf 100644 --- a/tests/ahriman/models/test_package.py +++ b/tests/ahriman/models/test_package.py @@ -135,24 +135,6 @@ def test_from_json_view_3(package_tpacpi_bat_git: Package) -> None: assert Package.from_json(package_tpacpi_bat_git.view()) == package_tpacpi_bat_git -def test_dependencies_with_version(mocker: MockerFixture, resource_path_root: Path) -> None: - """ - must load correct list of dependencies with version - """ - srcinfo = (resource_path_root / "models" / "package_yay_srcinfo").read_text() - mocker.patch("pathlib.Path.read_text", return_value=srcinfo) - - assert Package.dependencies(Path("path")) == {"git", "go", "pacman"} - - -def test_full_version() -> None: - """ - must construct full version - """ - assert Package.full_version("1", "r2388.d30e3201", "1") == "1:r2388.d30e3201-1" - assert Package.full_version(None, "0.12.1", "1") == "0.12.1-1" - - def test_load_from_archive(package_ahriman: Package, pyalpm_handle: MagicMock, mocker: MockerFixture) -> None: """ must load package from package archive @@ -198,6 +180,24 @@ def test_load_failure(package_ahriman: Package, pyalpm_handle: MagicMock, mocker Package.load(Path("path"), pyalpm_handle, package_ahriman.aur_url) +def test_dependencies_with_version(mocker: MockerFixture, resource_path_root: Path) -> None: + """ + must load correct list of dependencies with version + """ + srcinfo = (resource_path_root / "models" / "package_yay_srcinfo").read_text() + mocker.patch("pathlib.Path.read_text", return_value=srcinfo) + + assert Package.dependencies(Path("path")) == {"git", "go", "pacman"} + + +def test_full_version() -> None: + """ + must construct full version + """ + assert Package.full_version("1", "r2388.d30e3201", "1") == "1:r2388.d30e3201-1" + assert Package.full_version(None, "0.12.1", "1") == "0.12.1-1" + + def test_actual_version(package_ahriman: Package, repository_paths: RepositoryPaths) -> None: """ must return same actual_version as version is