chore: add metthod definition order plugin to pylint

Also reorder some methods to fix errors

.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.

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 <http://www.gnu.org/licenses/>.
+#
+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))

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

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

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:

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

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

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):

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

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

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

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

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):

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

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

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

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