From 04e5a263b73a30904ea1d2b3c13f462bf03b8b0d Mon Sep 17 00:00:00 2001 From: Evgeniy Alekseev Date: Tue, 3 Jan 2023 01:53:10 +0200 Subject: [PATCH] add notes about documentation and methods inside class Because I always forget which way I used before --- CONTRIBUTING.md | 90 ++++++++++++++++++- .../application/application_packages.py | 48 +++++----- .../application/application_repository.py | 24 ++--- src/ahriman/application/handlers/versions.py | 2 +- src/ahriman/application/lock.py | 68 +++++++------- src/ahriman/core/alpm/pacman.py | 38 ++++---- src/ahriman/core/formatters/printer.py | 3 +- src/ahriman/core/log/lazy_logging.py | 38 ++++---- src/ahriman/core/repository/cleaner.py | 24 ++--- src/ahriman/core/triggers/trigger_loader.py | 16 ++-- .../application/test_application_packages.py | 32 +++---- .../test_application_repository.py | 16 ++-- tests/ahriman/application/test_lock.py | 68 +++++++------- tests/ahriman/core/log/test_lazy_logging.py | 32 +++---- tests/ahriman/core/repository/test_cleaner.py | 16 ++-- 15 files changed, 301 insertions(+), 214 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8abab5b6..88ab1fcd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -26,7 +26,95 @@ In order to resolve all difficult cases the `autopep8` is used. You can perform Again, the most checks can be performed by `make check` command, though some additional guidelines must be applied: * Every class, every function (including private and protected), every attribute must be documented. The project follows [Google style documentation](https://google.github.io/styleguide/pyguide.html). The only exception is local functions. +* Correct way to document function, if section is empty, e.g. no notes or there are no args, it should be omitted: + + ```python + def foo(argument: str, *, flag: bool = False) -> int: + """ + do foo + + Note: + Very important note about this function + + Args: + argument(str): an argument + flag(bool, optional): a flag (Default value = False) + + Returns: + int: result + + Raises: + RuntimeException: a local function error occurs + + Examples: + Very informative example how to use this function, e.g.:: + + >>> foo("argument", flag=False) + + Note that function documentation is in rST. + """ + ``` + + `Returns` should be replaced with `Yields` for generators. + + Class attributes should be documented in the following way: + + ```python + class Clazz(BaseClazz): + """ + brand-new implementation of ``BaseClazz`` + + Attributes: + CLAZZ_ATTRIBUTE(int): (class attribute) a brand-new class attribute + instance_attribute(str): an instance attribute + + Examples: + Very informative class usage example, e.g.:: + + >>> from module import Clazz + >>> clazz = Clazz() + """ + + CLAZZ_ATTRIBUTE = 42 + + def __init__(self) -> None: + """ + default constructor + """ + self.instance_attribute = "" + ``` + * Type annotations are the must, even for local functions. +* Recommended order of function definitions in class: + + ```python + class Clazz: + + def __init__(self) -> None: ... # replace with `__post_init__` for dataclasses + + @property + def property(self) -> Any: ... + + @classmethod + def class_method(cls: Type[Clazz]) -> Clazz: ... + + @staticmethod + def static_method() -> Any: ... + + def __private_method(self) -> Any: ... + + def _protected_method(self) -> Any: ... + + def usual_method(self) -> Any: ... + + def __hash__(self) -> int: ... # basically any magic (or look-alike) method + ``` + + Methods inside one group should be ordered alphabetically, the only exception is `__init__` method (`__post__init__` for dataclasses) which should be defined first. For test methods it is recommended to follow the order in which functions are defined. + + Though, we would like to highlight abstract methods (i.e. ones which raise `NotImplementedError`), we still keep in global order at the moment. + +* Abstract methods must raise `NotImplementedError` instead of using `abc.abstractmethod`. The reason behind this restriction is the fact that we have class/static abstract methods for those we need to define their attribute first making the code harder to read. * For any path interactions `pathlib.Path` must be used. * Configuration interactions must go through `ahriman.core.configuration.Configuration` class instance. * In case if class load requires some actions, it is recommended to create class method which can be used for class instantiating. @@ -59,7 +147,7 @@ Again, the most checks can be performed by `make check` command, though some add * One file should define only one class, exception is class satellites in case if file length remains less than 400 lines. * It is possible to create file which contains some functions (e.g. `ahriman.core.util`), but in this case you would need to define `__all__` attribute. * The file size mentioned above must be applicable in general. In case of big classes consider splitting them into traits. Note, however, that `pylint` includes comments and docstrings into counter, thus you need to check file size by other tools. -* No global variable is allowed outside of `ahriman.version` module. +* No global variable is allowed outside of `ahriman.version` module. `ahriman.core.context` is also special case. * Single quotes are not allowed. The reason behind this restriction is the fact that docstrings must be written by using double quotes only, and we would like to make style consistent. * If your class writes anything to log, the `ahriman.core.log.LazyLogging` trait must be used. diff --git a/src/ahriman/application/application/application_packages.py b/src/ahriman/application/application/application_packages.py index 350332b5..39e6a5ba 100644 --- a/src/ahriman/application/application/application_packages.py +++ b/src/ahriman/application/application/application_packages.py @@ -37,30 +37,6 @@ class ApplicationPackages(ApplicationProperties): package control class """ - def _known_packages(self) -> Set[str]: - """ - load packages from repository and pacman repositories - - Returns: - Set[str]: list of known packages - - Raises: - NotImplementedError: not implemented method - """ - raise NotImplementedError - - def on_result(self, result: Result) -> None: - """ - generate report and sync to remote server - - Args: - result(Result): build result - - Raises: - NotImplementedError: not implemented method - """ - raise NotImplementedError - def _add_archive(self, source: str, *_: Any) -> None: """ add package from archive @@ -147,6 +123,18 @@ class ApplicationPackages(ApplicationProperties): self.database.remote_update(package) # repository packages must not depend on unknown packages, thus we are not going to process dependencies + def _known_packages(self) -> Set[str]: + """ + load packages from repository and pacman repositories + + Returns: + Set[str]: list of known packages + + Raises: + NotImplementedError: not implemented method + """ + raise NotImplementedError + def _process_dependencies(self, local_dir: Path, known_packages: Set[str], without_dependencies: bool) -> None: """ process package dependencies @@ -178,6 +166,18 @@ class ApplicationPackages(ApplicationProperties): fn = getattr(self, f"_add_{resolved_source.value}") fn(name, known_packages, without_dependencies) + def on_result(self, result: Result) -> None: + """ + generate report and sync to remote server + + Args: + result(Result): build result + + Raises: + NotImplementedError: not implemented method + """ + raise NotImplementedError + def remove(self, names: Iterable[str]) -> None: """ remove packages from repository diff --git a/src/ahriman/application/application/application_repository.py b/src/ahriman/application/application/application_repository.py index 61afa100..d0374879 100644 --- a/src/ahriman/application/application/application_repository.py +++ b/src/ahriman/application/application/application_repository.py @@ -33,18 +33,6 @@ class ApplicationRepository(ApplicationProperties): repository control class """ - def on_result(self, result: Result) -> None: - """ - generate report and sync to remote server - - Args: - result(Result): build result - - Raises: - NotImplementedError: not implemented method - """ - raise NotImplementedError - def clean(self, *, cache: bool, chroot: bool, manual: bool, packages: bool, pacman: bool) -> None: """ run all clean methods. Warning: some functions might not be available under non-root @@ -67,6 +55,18 @@ class ApplicationRepository(ApplicationProperties): if pacman: self.repository.clear_pacman() + def on_result(self, result: Result) -> None: + """ + generate report and sync to remote server + + Args: + result(Result): build result + + Raises: + NotImplementedError: not implemented method + """ + raise NotImplementedError + def sign(self, packages: Iterable[str]) -> None: """ sign packages and repository diff --git a/src/ahriman/application/handlers/versions.py b/src/ahriman/application/handlers/versions.py index c1d90693..0f57a55c 100644 --- a/src/ahriman/application/handlers/versions.py +++ b/src/ahriman/application/handlers/versions.py @@ -61,7 +61,7 @@ class Versions(Handler): Args: root(str): root package name - root_extras(Tuple[str, ...]): extras for the root package (Default value = ()) + root_extras(Tuple[str, ...], optional): extras for the root package (Default value = ()) Returns: Dict[str, str]: map of installed dependency to its version diff --git a/src/ahriman/application/lock.py b/src/ahriman/application/lock.py index 315e3ca4..3a6ada9a 100644 --- a/src/ahriman/application/lock.py +++ b/src/ahriman/application/lock.py @@ -75,40 +75,6 @@ class Lock(LazyLogging): self.paths = configuration.repository_paths self.reporter = Client.load(configuration, report=args.report) - def __enter__(self) -> Lock: - """ - default workflow is the following: - - 1. Check user UID - 2. Check if there is lock file - 3. Check web status watcher status - 4. Create lock file - 5. Report to status page if enabled - """ - self.check_user() - self.check_version() - self.create() - self.reporter.update_self(BuildStatusEnum.Building) - return self - - def __exit__(self, exc_type: Optional[Type[Exception]], exc_val: Optional[Exception], - exc_tb: TracebackType) -> Literal[False]: - """ - remove lock file when done - - Args: - exc_type(Optional[Type[Exception]]): exception type name if any - exc_val(Optional[Exception]): exception raised if any - exc_tb(TracebackType): exception traceback if any - - Returns: - Literal[False]: always False (do not suppress any exception) - """ - self.clear() - status = BuildStatusEnum.Success if exc_val is None else BuildStatusEnum.Failed - self.reporter.update_self(status) - return False - def check_version(self) -> None: """ check web server version @@ -145,3 +111,37 @@ class Lock(LazyLogging): self.path.touch(exist_ok=self.force) except FileExistsError: raise DuplicateRunError() + + def __enter__(self) -> Lock: + """ + default workflow is the following: + + 1. Check user UID + 2. Check if there is lock file + 3. Check web status watcher status + 4. Create lock file + 5. Report to status page if enabled + """ + self.check_user() + self.check_version() + self.create() + self.reporter.update_self(BuildStatusEnum.Building) + return self + + def __exit__(self, exc_type: Optional[Type[Exception]], exc_val: Optional[Exception], + exc_tb: TracebackType) -> Literal[False]: + """ + remove lock file when done + + Args: + exc_type(Optional[Type[Exception]]): exception type name if any + exc_val(Optional[Exception]): exception raised if any + exc_tb(TracebackType): exception traceback if any + + Returns: + Literal[False]: always False (do not suppress any exception) + """ + self.clear() + status = BuildStatusEnum.Success if exc_val is None else BuildStatusEnum.Failed + self.reporter.update_self(status) + return False diff --git a/src/ahriman/core/alpm/pacman.py b/src/ahriman/core/alpm/pacman.py index fb3c6311..f646cd0e 100644 --- a/src/ahriman/core/alpm/pacman.py +++ b/src/ahriman/core/alpm/pacman.py @@ -81,25 +81,6 @@ class Pacman(LazyLogging): return handle - def __getattr__(self, item: str) -> Any: - """ - pacman handle extractor - - Args: - item(str): property name - - Returns: - Any: attribute by its name - - Raises: - AttributeError: in case if no such attribute found - """ - if item == "handle": - handle = self.__create_handle_fn() - setattr(self, item, handle) - return handle - return super().__getattr__(item) # required for logging attribute - def database_copy(self, handle: Handle, database: DB, pacman_root: Path, paths: RepositoryPaths, *, use_ahriman_cache: bool) -> None: """ @@ -198,3 +179,22 @@ class Pacman(LazyLogging): result.update(package.provides) # provides list for meta-packages return result + + def __getattr__(self, item: str) -> Any: + """ + pacman handle extractor + + Args: + item(str): property name + + Returns: + Any: attribute by its name + + Raises: + AttributeError: in case if no such attribute found + """ + if item == "handle": + handle = self.__create_handle_fn() + setattr(self, item, handle) + return handle + return super().__getattr__(item) # required for logging attribute diff --git a/src/ahriman/core/formatters/printer.py b/src/ahriman/core/formatters/printer.py index 3bbd8403..b5ea619b 100644 --- a/src/ahriman/core/formatters/printer.py +++ b/src/ahriman/core/formatters/printer.py @@ -33,8 +33,7 @@ class Printer: Args: verbose(bool): print all fields - log_fn(Callable[[str]): logger function to log data - None]: (Default value = print) + log_fn(Callable[[str], None]): logger function to log data (Default value = print) separator(str, optional): separator for property name and property value (Default value = ": ") """ if (title := self.title()) is not None: diff --git a/src/ahriman/core/log/lazy_logging.py b/src/ahriman/core/log/lazy_logging.py index 841b4297..68502a07 100644 --- a/src/ahriman/core/log/lazy_logging.py +++ b/src/ahriman/core/log/lazy_logging.py @@ -33,25 +33,6 @@ class LazyLogging: logger: logging.Logger - def __getattr__(self, item: str) -> Any: - """ - logger extractor - - Args: - item(str): property name - - Returns: - Any: attribute by its name - - Raises: - AttributeError: in case if no such attribute found - """ - if item == "logger": - logger = logging.getLogger(self.logger_name) - setattr(self, item, logger) - return logger - raise AttributeError(f"'{self.__class__.__qualname__}' object has no attribute '{item}'") - @property def logger_name(self) -> str: """ @@ -107,3 +88,22 @@ class LazyLogging: yield finally: self._package_logger_reset() + + def __getattr__(self, item: str) -> Any: + """ + logger extractor + + Args: + item(str): property name + + Returns: + Any: attribute by its name + + Raises: + AttributeError: in case if no such attribute found + """ + if item == "logger": + logger = logging.getLogger(self.logger_name) + setattr(self, item, logger) + return logger + raise AttributeError(f"'{self.__class__.__qualname__}' object has no attribute '{item}'") diff --git a/src/ahriman/core/repository/cleaner.py b/src/ahriman/core/repository/cleaner.py index 49ae4b61..8ce2473b 100644 --- a/src/ahriman/core/repository/cleaner.py +++ b/src/ahriman/core/repository/cleaner.py @@ -30,18 +30,6 @@ class Cleaner(RepositoryProperties): trait to clean common repository objects """ - def packages_built(self) -> List[Path]: - """ - get list of files in built packages directory - - Returns: - List[Path]: list of filenames from the directory - - Raises: - NotImplementedError: not implemented method - """ - raise NotImplementedError - def clear_cache(self) -> None: """ clear cache directory @@ -80,3 +68,15 @@ class Cleaner(RepositoryProperties): """ self.logger.info("clear build queue") self.database.build_queue_clear(None) + + def packages_built(self) -> List[Path]: + """ + get list of files in built packages directory + + Returns: + List[Path]: list of filenames from the directory + + Raises: + NotImplementedError: not implemented method + """ + raise NotImplementedError diff --git a/src/ahriman/core/triggers/trigger_loader.py b/src/ahriman/core/triggers/trigger_loader.py index ab833bb6..8a8fbee9 100644 --- a/src/ahriman/core/triggers/trigger_loader.py +++ b/src/ahriman/core/triggers/trigger_loader.py @@ -75,14 +75,6 @@ class TriggerLoader(LazyLogging): for trigger in configuration.getlist("build", "triggers") ] - def __del__(self) -> None: - """ - custom destructor object which calls on_stop in case if it was requested - """ - if not self._on_stop_requested: - return - self.on_stop() - @contextlib.contextmanager def __execute_trigger(self, trigger: Trigger) -> Generator[None, None, None]: """ @@ -206,3 +198,11 @@ class TriggerLoader(LazyLogging): for trigger in self.triggers: with self.__execute_trigger(trigger): trigger.on_stop() + + def __del__(self) -> None: + """ + custom destructor object which calls on_stop in case if it was requested + """ + if not self._on_stop_requested: + return + self.on_stop() diff --git a/tests/ahriman/application/application/test_application_packages.py b/tests/ahriman/application/application/test_application_packages.py index 6bb77f4a..b2c8f971 100644 --- a/tests/ahriman/application/application/test_application_packages.py +++ b/tests/ahriman/application/application/test_application_packages.py @@ -11,22 +11,6 @@ from ahriman.models.package_source import PackageSource from ahriman.models.result import Result -def test_on_result(application_packages: ApplicationPackages) -> None: - """ - must raise NotImplemented for missing finalize method - """ - with pytest.raises(NotImplementedError): - application_packages.on_result(Result()) - - -def test_known_packages(application_packages: ApplicationPackages) -> None: - """ - must raise NotImplemented for missing known_packages method - """ - with pytest.raises(NotImplementedError): - application_packages._known_packages() - - def test_add_archive( application_packages: ApplicationPackages, package_ahriman: Package, @@ -129,6 +113,14 @@ def test_add_repository(application_packages: ApplicationPackages, package_ahrim update_remote_mock.assert_called_once_with(package_ahriman) +def test_known_packages(application_packages: ApplicationPackages) -> None: + """ + must raise NotImplemented for missing known_packages method + """ + with pytest.raises(NotImplementedError): + application_packages._known_packages() + + def test_process_dependencies(application_packages: ApplicationPackages, mocker: MockerFixture) -> None: """ must process dependencies addition @@ -237,6 +229,14 @@ def test_add_add_remote(application_packages: ApplicationPackages, package_descr add_mock.assert_called_once_with(url, set(), False) +def test_on_result(application_packages: ApplicationPackages) -> None: + """ + must raise NotImplemented for missing finalize method + """ + with pytest.raises(NotImplementedError): + application_packages.on_result(Result()) + + def test_remove(application_packages: ApplicationPackages, mocker: MockerFixture) -> None: """ must remove package diff --git a/tests/ahriman/application/application/test_application_repository.py b/tests/ahriman/application/application/test_application_repository.py index 54aa9bf5..e6c5456c 100644 --- a/tests/ahriman/application/application/test_application_repository.py +++ b/tests/ahriman/application/application/test_application_repository.py @@ -9,14 +9,6 @@ from ahriman.models.package import Package from ahriman.models.result import Result -def test_on_result(application_repository: ApplicationRepository) -> None: - """ - must raise NotImplemented for missing finalize method - """ - with pytest.raises(NotImplementedError): - application_repository.on_result(Result()) - - def test_clean_cache(application_repository: ApplicationRepository, mocker: MockerFixture) -> None: """ must clean cache directory @@ -62,6 +54,14 @@ def test_clean_pacman(application_repository: ApplicationRepository, mocker: Moc clear_mock.assert_called_once_with() +def test_on_result(application_repository: ApplicationRepository) -> None: + """ + must raise NotImplemented for missing finalize method + """ + with pytest.raises(NotImplementedError): + application_repository.on_result(Result()) + + def test_sign(application_repository: ApplicationRepository, package_ahriman: Package, package_python_schedule: Package, mocker: MockerFixture) -> None: """ diff --git a/tests/ahriman/application/test_lock.py b/tests/ahriman/application/test_lock.py index cbf3f320..21e7917f 100644 --- a/tests/ahriman/application/test_lock.py +++ b/tests/ahriman/application/test_lock.py @@ -12,40 +12,6 @@ from ahriman.models.build_status import BuildStatus, BuildStatusEnum from ahriman.models.internal_status import InternalStatus -def test_enter(lock: Lock, mocker: MockerFixture) -> None: - """ - must process with context manager - """ - check_user_mock = mocker.patch("ahriman.application.lock.Lock.check_user") - check_version_mock = mocker.patch("ahriman.application.lock.Lock.check_version") - 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_with() - clear_mock.assert_called_once_with() - create_mock.assert_called_once_with() - check_version_mock.assert_called_once_with() - update_status_mock.assert_has_calls([MockCall(BuildStatusEnum.Building), MockCall(BuildStatusEnum.Success)]) - - -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.clear") - mocker.patch("ahriman.application.lock.Lock.create") - update_status_mock = mocker.patch("ahriman.core.status.client.Client.update_self") - - with pytest.raises(Exception): - with lock: - raise Exception() - update_status_mock.assert_has_calls([MockCall(BuildStatusEnum.Building), MockCall(BuildStatusEnum.Failed)]) - - def test_check_version(lock: Lock, mocker: MockerFixture) -> None: """ must check version correctly @@ -166,3 +132,37 @@ def test_create_unsafe(lock: Lock) -> None: lock.create() lock.path.unlink() + + +def test_enter(lock: Lock, mocker: MockerFixture) -> None: + """ + must process with context manager + """ + check_user_mock = mocker.patch("ahriman.application.lock.Lock.check_user") + check_version_mock = mocker.patch("ahriman.application.lock.Lock.check_version") + 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_with() + clear_mock.assert_called_once_with() + create_mock.assert_called_once_with() + check_version_mock.assert_called_once_with() + update_status_mock.assert_has_calls([MockCall(BuildStatusEnum.Building), MockCall(BuildStatusEnum.Success)]) + + +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.clear") + mocker.patch("ahriman.application.lock.Lock.create") + update_status_mock = mocker.patch("ahriman.core.status.client.Client.update_self") + + with pytest.raises(Exception): + with lock: + raise Exception() + update_status_mock.assert_has_calls([MockCall(BuildStatusEnum.Building), MockCall(BuildStatusEnum.Failed)]) diff --git a/tests/ahriman/core/log/test_lazy_logging.py b/tests/ahriman/core/log/test_lazy_logging.py index 15628894..fd4e030d 100644 --- a/tests/ahriman/core/log/test_lazy_logging.py +++ b/tests/ahriman/core/log/test_lazy_logging.py @@ -8,22 +8,6 @@ from ahriman.core.database import SQLite from ahriman.models.package import Package -def test_logger(database: SQLite) -> None: - """ - must set logger attribute - """ - assert database.logger - assert database.logger.name == "ahriman.core.database.sqlite.SQLite" - - -def test_logger_attribute_error(database: SQLite) -> None: - """ - must raise AttributeError in case if no attribute found - """ - with pytest.raises(AttributeError): - database.loggerrrr - - def test_logger_name(database: SQLite, repo: Repo) -> None: """ must correctly generate logger name @@ -74,3 +58,19 @@ def test_in_package_context_failed(database: SQLite, package_ahriman: Package, m raise Exception() reset_mock.assert_called_once_with() + + +def test_logger(database: SQLite) -> None: + """ + must set logger attribute + """ + assert database.logger + assert database.logger.name == "ahriman.core.database.sqlite.SQLite" + + +def test_logger_attribute_error(database: SQLite) -> None: + """ + must raise AttributeError in case if no attribute found + """ + with pytest.raises(AttributeError): + database.loggerrrr diff --git a/tests/ahriman/core/repository/test_cleaner.py b/tests/ahriman/core/repository/test_cleaner.py index eaf02d22..4a9cf5ac 100644 --- a/tests/ahriman/core/repository/test_cleaner.py +++ b/tests/ahriman/core/repository/test_cleaner.py @@ -30,14 +30,6 @@ def _mock_clear_check() -> None: ]) -def test_packages_built(cleaner: Cleaner) -> None: - """ - must raise NotImplemented for missing method - """ - with pytest.raises(NotImplementedError): - cleaner.packages_built() - - def test_clear_cache(cleaner: Cleaner, mocker: MockerFixture) -> None: """ must remove every cached sources @@ -84,3 +76,11 @@ def test_clear_queue(cleaner: Cleaner, mocker: MockerFixture) -> None: clear_mock = mocker.patch("ahriman.core.database.SQLite.build_queue_clear") cleaner.clear_queue() clear_mock.assert_called_once_with(None) + + +def test_packages_built(cleaner: Cleaner) -> None: + """ + must raise NotImplemented for missing method + """ + with pytest.raises(NotImplementedError): + cleaner.packages_built()