From df787657aaf2714aa9a4b3793b27433b4d327517 Mon Sep 17 00:00:00 2001 From: Evgenii Alekseev Date: Sat, 4 Nov 2023 16:16:14 +0200 Subject: [PATCH] chore: add metthod definition order plugin to pylint Also reorder some methods to fix errors --- .pylintrc | 7 +- pylint_plugins/definition_order.py | 171 ++++++++++++++++++ src/ahriman/application/ahriman.py | 40 ++-- src/ahriman/application/handlers/versions.py | 4 +- src/ahriman/application/handlers/web.py | 4 +- src/ahriman/application/lock.py | 14 +- src/ahriman/core/formatters/printer.py | 1 + .../core/log/filtered_access_logger.py | 2 +- src/ahriman/core/upload/github.py | 30 +-- src/ahriman/core/upload/s3.py | 28 +-- src/ahriman/models/package.py | 6 +- src/ahriman/models/repository_id.py | 20 +- src/ahriman/web/routes.py | 4 +- tests/ahriman/application/test_ahriman.py | 60 +++--- tests/ahriman/application/test_lock.py | 48 ++--- tests/ahriman/core/upload/test_github.py | 18 +- tests/ahriman/models/test_repository_id.py | 16 +- 17 files changed, 323 insertions(+), 150 deletions(-) create mode 100644 pylint_plugins/definition_order.py diff --git a/.pylintrc b/.pylintrc index 89b7efae..9ce72fd9 100644 --- a/.pylintrc +++ b/.pylintrc @@ -67,7 +67,7 @@ ignored-modules= # Python code to execute, usually for sys.path manipulation such as # pygtk.require(). -#init-hook= +init-hook='import sys; sys.path.append("pylint_plugins")' # Use multiple processes to speed up Pylint. Specifying 0 will auto-detect the # number of processors available to use, and will cap the count on Windows to @@ -81,7 +81,8 @@ limit-inference-results=100 # List of plugins (as comma separated values of python module names) to load, # usually to register additional checkers. -load-plugins= +load-plugins=pylint.extensions.docparams, + definition_order, # Pickle collected data for later comparisons. persistent=yes @@ -227,7 +228,7 @@ name-group= # Regular expression which should only match function or class names that do # not require a docstring. -no-docstring-rgx=^_ +no-docstring-rgx= # List of decorators that produce properties, such as abc.abstractproperty. Add # to this list to register other decorators that produce valid properties. diff --git a/pylint_plugins/definition_order.py b/pylint_plugins/definition_order.py new file mode 100644 index 00000000..7d9dcd77 --- /dev/null +++ b/pylint_plugins/definition_order.py @@ -0,0 +1,171 @@ +# +# Copyright (c) 2021-2023 ahriman team. +# +# This file is part of ahriman +# (see https://github.com/arcan1s/ahriman). +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# +from astroid import nodes +from collections.abc import Iterable +from enum import Enum +from pylint.checkers import BaseRawFileChecker +from pylint.lint import PyLinter +from typing import Any + + +class MethodTypeOrder(int, Enum): + """ + method type enumeration + + Attributes: + New(MethodTypeOrder): (class attribute) constructor method + Init(MethodTypeOrder): (class attribute) initialization method + Property(MethodTypeOrder): (class attribute) property method + Class(MethodTypeOrder): (class attribute) class method + Static(MethodTypeOrder): (class attribute) static method + Normal(MethodTypeOrder): (class attribute) usual method + Magic(MethodTypeOrder): (class attribute) other magical methods + """ + + New = 0 + Init = 1 + Property = 2 + Class = 3 + Static = 4 + Normal = 5 + Magic = 6 + + +class DefinitionOrder(BaseRawFileChecker): + """ + check if methods are defined in recommended order + + Attributes: + DECORATED_METHODS_ORDER(list[str]): (class attribute) predefined list of known function decorators + """ + + DECORATED_METHODS_ORDER = { + "cached_property": MethodTypeOrder.Property, + "classmethod": MethodTypeOrder.Class, + "property": MethodTypeOrder.Property, + "staticmethod": MethodTypeOrder.Static, + } + + name = "method-ordering" + msgs = { + "W0001": ( + "Invalid method order %s, expected %s", + "methods-out-of-order", + "Methods are defined out of recommended order.", + ) + } + options = () + + @staticmethod + def comparator(function: nodes.FunctionDef) -> tuple[int, str]: + """ + compare key for function node + + Args: + function(nodes.FunctionDef): function definition + + Returns: + tuple[int, str]: comparison key + """ + # init methods + if function.name in ("__new__",): + return MethodTypeOrder.New, function.name + if function.name in ("__init__", "__post_init__"): + return MethodTypeOrder.Init, function.name + + # decorated methods + decorators = [] + if function.decorators is not None: + decorators = [getattr(decorator, "name", None) for decorator in function.decorators.get_children()] + for decorator in decorators: + if decorator in DefinitionOrder.DECORATED_METHODS_ORDER: + return DefinitionOrder.DECORATED_METHODS_ORDER[decorator], function.name + + # magic methods + if function.name.startswith("__") and function.name.endswith("__"): + return MethodTypeOrder.Magic, function.name + + # normal method + return MethodTypeOrder.Normal, function.name + + @staticmethod + def methods(source: Iterable[Any], start_lineno: int = 0) -> list[nodes.FunctionDef]: + """ + extract function nodes from list of raw nodes + + Args: + source(Iterable[Any]): all available nodes + start_lineno(int, optional): minimal allowed line number (Default value = 0) + + Returns: + list[nodes.FunctionDef]: list of function nodes + """ + def is_defined_function(function: Any) -> bool: + return isinstance(function, nodes.FunctionDef) \ + and function.lineno is not None \ + and function.lineno >= start_lineno + + return list(filter(is_defined_function, source)) + + def check_class(self, clazz: nodes.ClassDef) -> None: + """ + check class functions ordering + + Args: + clazz(nodes.ClassDef): class definition + """ + methods = DefinitionOrder.methods(clazz.values(), clazz.lineno) + self.check_functions(methods) + + def check_functions(self, functions: list[nodes.FunctionDef]) -> None: + """ + check global functions ordering + + Args: + functions(list[nodes.FunctionDef]): list of functions in their defined order + """ + for real, expected in zip(functions, sorted(functions, key=DefinitionOrder.comparator)): + if real == expected: + continue + self.add_message("methods-out-of-order", line=real.lineno, args=(real.name, expected.name)) + break + + def process_module(self, node: nodes.Module) -> None: + """ + process module + + Args: + node(nodes.Module): module node to check + """ + # check global methods + self.check_functions(DefinitionOrder.methods(node.values())) + # check class definitions + for clazz in filter(lambda method: isinstance(method, nodes.ClassDef), node.values()): + self.check_class(clazz) + + +def register(linter: PyLinter) -> None: + """ + register custom checker + + Args: + linter(PyLinter): linter in which checker should be registered + """ + linter.register_checker(DefinitionOrder(linter)) diff --git a/src/ahriman/application/ahriman.py b/src/ahriman/application/ahriman.py index f29f1749..67456a27 100644 --- a/src/ahriman/application/ahriman.py +++ b/src/ahriman/application/ahriman.py @@ -96,8 +96,8 @@ def _parser() -> argparse.ArgumentParser: subparsers = parser.add_subparsers(title="command", help="command to run", dest="command") _set_aur_search_parser(subparsers) - _set_help_parser(subparsers) _set_help_commands_unsafe_parser(subparsers) + _set_help_parser(subparsers) _set_help_updates_parser(subparsers) _set_help_version_parser(subparsers) _set_package_add_parser(subparsers) @@ -166,25 +166,6 @@ def _set_aur_search_parser(root: SubParserAction) -> argparse.ArgumentParser: return parser -def _set_help_parser(root: SubParserAction) -> argparse.ArgumentParser: - """ - add parser for listing help subcommand - - Args: - root(SubParserAction): subparsers for the commands - - Returns: - argparse.ArgumentParser: created argument parser - """ - parser = root.add_parser("help", help="show help message", - description="show help message for application or command and exit", - formatter_class=_formatter) - parser.add_argument("command", help="show help message for specific command", nargs="?") - parser.set_defaults(handler=handlers.Help, architecture="", lock=None, quiet=True, report=False, repository="", - unsafe=True, parser=_parser) - return parser - - def _set_help_commands_unsafe_parser(root: SubParserAction) -> argparse.ArgumentParser: """ add parser for listing unsafe commands @@ -204,6 +185,25 @@ def _set_help_commands_unsafe_parser(root: SubParserAction) -> argparse.Argument return parser +def _set_help_parser(root: SubParserAction) -> argparse.ArgumentParser: + """ + add parser for listing help subcommand + + Args: + root(SubParserAction): subparsers for the commands + + Returns: + argparse.ArgumentParser: created argument parser + """ + parser = root.add_parser("help", help="show help message", + description="show help message for application or command and exit", + formatter_class=_formatter) + parser.add_argument("command", help="show help message for specific command", nargs="?") + parser.set_defaults(handler=handlers.Help, architecture="", lock=None, quiet=True, report=False, repository="", + unsafe=True, parser=_parser) + return parser + + def _set_help_updates_parser(root: SubParserAction) -> argparse.ArgumentParser: """ add parser for service update check subcommand diff --git a/src/ahriman/application/handlers/versions.py b/src/ahriman/application/handlers/versions.py index 8dfc9c0a..84008da9 100644 --- a/src/ahriman/application/handlers/versions.py +++ b/src/ahriman/application/handlers/versions.py @@ -67,8 +67,8 @@ class Versions(Handler): Args: root(str): root package name - Returns: - Generator[tuple[str, str], None, None]: map of installed dependency to its version + Yields: + tuple[str, str]: map of installed dependency to its version """ def dependencies_by_key(key: str) -> Generator[str, None, None]: # in importlib it returns requires in the following format diff --git a/src/ahriman/application/handlers/web.py b/src/ahriman/application/handlers/web.py index f35f0516..7baaaf70 100644 --- a/src/ahriman/application/handlers/web.py +++ b/src/ahriman/application/handlers/web.py @@ -76,8 +76,8 @@ class Web(Handler): args(argparse.Namespace): command line args configuration(Configuration): configuration instance - Returns: - Generator[str, None, None]: command line arguments which were used for this specific command + Yields: + str: command line arguments which were used for this specific command """ # read configuration path from current settings if (configuration_path := configuration.path) is not None: diff --git a/src/ahriman/application/lock.py b/src/ahriman/application/lock.py index 0143d2c9..7f4f2d7c 100644 --- a/src/ahriman/application/lock.py +++ b/src/ahriman/application/lock.py @@ -80,6 +80,13 @@ class Lock(LazyLogging): self.paths = configuration.repository_paths self.reporter = Client.load(repository_id, configuration, report=args.report) + def check_user(self) -> None: + """ + check if current user is actually owner of ahriman root + """ + check_user(self.paths, unsafe=self.unsafe) + self.paths.tree_create() + def check_version(self) -> None: """ check web server version @@ -89,13 +96,6 @@ class Lock(LazyLogging): self.logger.warning("status watcher version mismatch, our %s, their %s", __version__, status.version) - def check_user(self) -> None: - """ - check if current user is actually owner of ahriman root - """ - check_user(self.paths, unsafe=self.unsafe) - self.paths.tree_create() - def clear(self) -> None: """ remove lock file diff --git a/src/ahriman/core/formatters/printer.py b/src/ahriman/core/formatters/printer.py index 4d147b05..00daf968 100644 --- a/src/ahriman/core/formatters/printer.py +++ b/src/ahriman/core/formatters/printer.py @@ -57,6 +57,7 @@ class Printer: """ return [] + # pylint: disable=redundant-returns-doc def title(self) -> str | None: """ generate entry title from content diff --git a/src/ahriman/core/log/filtered_access_logger.py b/src/ahriman/core/log/filtered_access_logger.py index 9d72859e..77bf3224 100644 --- a/src/ahriman/core/log/filtered_access_logger.py +++ b/src/ahriman/core/log/filtered_access_logger.py @@ -67,7 +67,7 @@ class FilteredAccessLogger(AccessLogger): Args: request(BaseRequest): http reqeust descriptor response(StreamResponse): streaming response object - time(float): + time(float): log record timestamp """ if self.is_logs_post(request) \ or self.is_process_get(request): diff --git a/src/ahriman/core/upload/github.py b/src/ahriman/core/upload/github.py index 35be8fa1..5cb25245 100644 --- a/src/ahriman/core/upload/github.py +++ b/src/ahriman/core/upload/github.py @@ -95,21 +95,6 @@ class GitHub(Upload, HttpUpload): with path.open("rb") as archive: self.make_request("POST", url, params=[("name", path.name)], data=archive, headers=headers) - def get_local_files(self, path: Path) -> dict[Path, str]: - """ - get all local files and their calculated checksums - - Args: - path(Path): local path to sync - - Returns: - dict[Path, str]: map of path objects to its checksum - """ - return { - local_file: self.calculate_hash(local_file) - for local_file in walk(path) - } - def files_remove(self, release: dict[str, Any], local_files: dict[Path, str], remote_files: dict[str, str]) -> None: """ remove files from GitHub @@ -140,6 +125,21 @@ class GitHub(Upload, HttpUpload): continue self.asset_upload(release, local_file) + def get_local_files(self, path: Path) -> dict[Path, str]: + """ + get all local files and their calculated checksums + + Args: + path(Path): local path to sync + + Returns: + dict[Path, str]: map of path objects to its checksum + """ + return { + local_file: self.calculate_hash(local_file) + for local_file in walk(path) + } + def release_create(self) -> dict[str, Any]: """ create empty release diff --git a/src/ahriman/core/upload/s3.py b/src/ahriman/core/upload/s3.py index 9ef115e2..1cd75678 100644 --- a/src/ahriman/core/upload/s3.py +++ b/src/ahriman/core/upload/s3.py @@ -87,6 +87,20 @@ class S3(Upload): suffix = f"-{len(md5s)}" if len(md5s) > 1 else "" return f"{checksum.hexdigest()}{suffix}" + @staticmethod + def files_remove(local_files: dict[Path, str], remote_objects: dict[Path, Any]) -> None: + """ + remove files which have been removed locally + + Args: + local_files(dict[Path, str]): map of local path object to its checksum + remote_objects(dict[Path, Any]): map of remote path object to the remote s3 object + """ + for local_file, remote_object in remote_objects.items(): + if local_file in local_files: + continue + remote_object.delete() + @staticmethod def get_bucket(configuration: Configuration, section: str) -> Any: """ @@ -105,20 +119,6 @@ class S3(Upload): aws_secret_access_key=configuration.get(section, "secret_key")) return client.Bucket(configuration.get(section, "bucket")) - @staticmethod - def files_remove(local_files: dict[Path, str], remote_objects: dict[Path, Any]) -> None: - """ - remove files which have been removed locally - - Args: - local_files(dict[Path, str]): map of local path object to its checksum - remote_objects(dict[Path, Any]): map of remote path object to the remote s3 object - """ - for local_file, remote_object in remote_objects.items(): - if local_file in local_files: - continue - remote_object.delete() - def files_upload(self, path: Path, local_files: dict[Path, str], remote_objects: dict[Path, Any]) -> None: """ upload changed files to s3 diff --git a/src/ahriman/models/package.py b/src/ahriman/models/package.py index 8e33572d..bc6b9b0d 100644 --- a/src/ahriman/models/package.py +++ b/src/ahriman/models/package.py @@ -354,9 +354,9 @@ class Package(LazyLogging): Args: path(Path): path to package sources directory - Returns: - Generator[Path, None, None]: list of paths of files which belong to the package and distributed together - with this tarball. All paths are relative to the ``path`` + Yields: + Path: list of paths of files which belong to the package and distributed together with this tarball. + All paths are relative to the ``path`` Raises: PackageInfoError: if there are parsing errors diff --git a/src/ahriman/models/repository_id.py b/src/ahriman/models/repository_id.py index 541761f2..545d3a8d 100644 --- a/src/ahriman/models/repository_id.py +++ b/src/ahriman/models/repository_id.py @@ -34,16 +34,6 @@ class RepositoryId: architecture: str name: str - @property - def is_empty(self) -> bool: - """ - check if all data is supplied for the loading - - Returns: - bool: True in case if architecture or name are not set and False otherwise - """ - return not self.architecture or not self.name - @property def id(self) -> str: """ @@ -56,6 +46,16 @@ class RepositoryId: return "" return f"{self.architecture}-{self.name}" # basically the same as used for command line + @property + def is_empty(self) -> bool: + """ + check if all data is supplied for the loading + + Returns: + bool: True in case if architecture or name are not set and False otherwise + """ + return not self.architecture or not self.name + def query(self) -> list[tuple[str, str]]: """ generate query parameters diff --git a/src/ahriman/web/routes.py b/src/ahriman/web/routes.py index 1c080676..a8ee94eb 100644 --- a/src/ahriman/web/routes.py +++ b/src/ahriman/web/routes.py @@ -41,8 +41,8 @@ def _dynamic_routes(module_root: Path) -> dict[str, Type[View]]: Returns: dict[str, Type[View]]: map of the route to its view """ - def is_base_view(clz: Any) -> TypeGuard[Type[BaseView]]: - return isinstance(clz, type) and issubclass(clz, BaseView) + def is_base_view(clazz: Any) -> TypeGuard[Type[BaseView]]: + return isinstance(clazz, type) and issubclass(clazz, BaseView) routes: dict[str, Type[View]] = {} for module_info in _modules(module_root): diff --git a/tests/ahriman/application/test_ahriman.py b/tests/ahriman/application/test_ahriman.py index 91b0060f..9c7aacbe 100644 --- a/tests/ahriman/application/test_ahriman.py +++ b/tests/ahriman/application/test_ahriman.py @@ -104,36 +104,6 @@ def test_subparsers_aur_search_option_repository(parser: argparse.ArgumentParser assert args.repository == "" -def test_subparsers_help(parser: argparse.ArgumentParser) -> None: - """ - help command must imply architecture list, lock, quiet, report, repository, unsafe and parser - """ - args = parser.parse_args(["help"]) - assert args.architecture == "" - assert args.lock is None - assert args.quiet - assert not args.report - assert args.repository == "" - assert args.unsafe - assert args.parser is not None and args.parser() - - -def test_subparsers_help_option_architecture(parser: argparse.ArgumentParser) -> None: - """ - help command must correctly parse architecture list - """ - args = parser.parse_args(["-a", "x86_64", "help"]) - assert args.architecture == "" - - -def test_subparsers_help_option_repository(parser: argparse.ArgumentParser) -> None: - """ - help command must correctly parse repository list - """ - args = parser.parse_args(["-r", "repo", "help"]) - assert args.repository == "" - - def test_subparsers_help_commands_unsafe(parser: argparse.ArgumentParser) -> None: """ help-commands-unsafe command must imply architecture list, lock, quiet, report, repository, unsafe and parser @@ -164,6 +134,36 @@ def test_subparsers_help_commands_unsafe_option_repository(parser: argparse.Argu assert args.repository == "" +def test_subparsers_help(parser: argparse.ArgumentParser) -> None: + """ + help command must imply architecture list, lock, quiet, report, repository, unsafe and parser + """ + args = parser.parse_args(["help"]) + assert args.architecture == "" + assert args.lock is None + assert args.quiet + assert not args.report + assert args.repository == "" + assert args.unsafe + assert args.parser is not None and args.parser() + + +def test_subparsers_help_option_architecture(parser: argparse.ArgumentParser) -> None: + """ + help command must correctly parse architecture list + """ + args = parser.parse_args(["-a", "x86_64", "help"]) + assert args.architecture == "" + + +def test_subparsers_help_option_repository(parser: argparse.ArgumentParser) -> None: + """ + help command must correctly parse repository list + """ + args = parser.parse_args(["-r", "repo", "help"]) + assert args.repository == "" + + def test_subparsers_help_updates(parser: argparse.ArgumentParser) -> None: """ help-updates command must imply architecture list, lock, quiet, report, repository, and unsafe diff --git a/tests/ahriman/application/test_lock.py b/tests/ahriman/application/test_lock.py index 1f787c4d..0375a5b6 100644 --- a/tests/ahriman/application/test_lock.py +++ b/tests/ahriman/application/test_lock.py @@ -30,30 +30,6 @@ def test_path(args: argparse.Namespace, configuration: Configuration) -> None: Lock(args, repository_id, configuration).path # special case -def test_check_version(lock: Lock, mocker: MockerFixture) -> None: - """ - must check version correctly - """ - mocker.patch("ahriman.core.status.client.Client.status_get", - return_value=InternalStatus(status=BuildStatus(), version=__version__)) - logging_mock = mocker.patch("logging.Logger.warning") - - lock.check_version() - logging_mock.assert_not_called() - - -def test_check_version_mismatch(lock: Lock, mocker: MockerFixture) -> None: - """ - must check mismatched version correctly - """ - mocker.patch("ahriman.core.status.client.Client.status_get", - return_value=InternalStatus(status=BuildStatus(), version="version")) - logging_mock = mocker.patch("logging.Logger.warning") - - lock.check_version() - logging_mock.assert_called_once() # we do not check logging arguments - - def test_check_user(lock: Lock, mocker: MockerFixture) -> None: """ must check user correctly @@ -84,6 +60,30 @@ def test_check_user_unsafe(lock: Lock, mocker: MockerFixture) -> None: lock.check_user() +def test_check_version(lock: Lock, mocker: MockerFixture) -> None: + """ + must check version correctly + """ + mocker.patch("ahriman.core.status.client.Client.status_get", + return_value=InternalStatus(status=BuildStatus(), version=__version__)) + logging_mock = mocker.patch("logging.Logger.warning") + + lock.check_version() + logging_mock.assert_not_called() + + +def test_check_version_mismatch(lock: Lock, mocker: MockerFixture) -> None: + """ + must check mismatched version correctly + """ + mocker.patch("ahriman.core.status.client.Client.status_get", + return_value=InternalStatus(status=BuildStatus(), version="version")) + logging_mock = mocker.patch("logging.Logger.warning") + + lock.check_version() + logging_mock.assert_called_once() # we do not check logging arguments + + def test_clear(lock: Lock) -> None: """ must remove lock file diff --git a/tests/ahriman/core/upload/test_github.py b/tests/ahriman/core/upload/test_github.py index e050b973..06afcfe1 100644 --- a/tests/ahriman/core/upload/test_github.py +++ b/tests/ahriman/core/upload/test_github.py @@ -89,15 +89,6 @@ def test_asset_upload_empty_mimetype(github: GitHub, github_release: dict[str, A headers={"Content-Type": "application/octet-stream"}) -def test_get_local_files(github: GitHub, resource_path_root: Path, mocker: MockerFixture) -> None: - """ - must get all local files recursively - """ - walk_mock = mocker.patch("ahriman.core.util.walk") - github.get_local_files(resource_path_root) - walk_mock.assert_called() - - def test_files_remove(github: GitHub, github_release: dict[str, Any], mocker: MockerFixture) -> None: """ must remove files from the remote @@ -137,6 +128,15 @@ def test_files_upload_empty(github: GitHub, github_release: dict[str, Any], mock upload_mock.assert_not_called() +def test_get_local_files(github: GitHub, resource_path_root: Path, mocker: MockerFixture) -> None: + """ + must get all local files recursively + """ + walk_mock = mocker.patch("ahriman.core.util.walk") + github.get_local_files(resource_path_root) + walk_mock.assert_called() + + def test_release_create(github: GitHub, mocker: MockerFixture) -> None: """ must create release diff --git a/tests/ahriman/models/test_repository_id.py b/tests/ahriman/models/test_repository_id.py index 99795958..94a9fb23 100644 --- a/tests/ahriman/models/test_repository_id.py +++ b/tests/ahriman/models/test_repository_id.py @@ -3,6 +3,14 @@ import pytest from ahriman.models.repository_id import RepositoryId +def test_id() -> None: + """ + must correctly generate id + """ + assert RepositoryId("", "").id == "" + assert RepositoryId("arch", "repo").id == "arch-repo" + + def test_is_empty() -> None: """ must check if repository id is empty or not @@ -13,14 +21,6 @@ def test_is_empty() -> None: assert not RepositoryId("arch", "repo").is_empty -def test_id() -> None: - """ - must correctly generate id - """ - assert RepositoryId("", "").id == "" - assert RepositoryId("arch", "repo").id == "arch-repo" - - def test_query() -> None: """ must generate query request parameters