Re: [PATCH v7 06/21] dts: logger and utils docstring update

[Yoan Picchi <yoan.picchi@foss.arm.com>]

On 11/15/23 13:09, Juraj Linkeš wrote:
> Format according to the Google format and PEP257, with slight
> deviations.
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> ---
>   dts/framework/logger.py | 72 ++++++++++++++++++++++-----------
>   dts/framework/utils.py  | 88 +++++++++++++++++++++++++++++------------
>   2 files changed, 113 insertions(+), 47 deletions(-)
> 
> diff --git a/dts/framework/logger.py b/dts/framework/logger.py
> index bb2991e994..d3eb75a4e4 100644
> --- a/dts/framework/logger.py
> +++ b/dts/framework/logger.py
> @@ -3,9 +3,9 @@
>   # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
>   # Copyright(c) 2022-2023 University of New Hampshire
>   
> -"""
> -DTS logger module with several log level. DTS framework and TestSuite logs
> -are saved in different log files.
> +"""DTS logger module.
> +
> +DTS framework and TestSuite logs are saved in different log files.
>   """
>   
>   import logging
> @@ -18,19 +18,21 @@
>   stream_fmt = "%(asctime)s - %(name)s - %(levelname)s - %(message)s"
>   
>   
> -class LoggerDictType(TypedDict):
> -    logger: "DTSLOG"
> -    name: str
> -    node: str
> -
> +class DTSLOG(logging.LoggerAdapter):
> +    """DTS logger adapter class for framework and testsuites.
>   
> -# List for saving all using loggers
> -Loggers: list[LoggerDictType] = []
> +    The :option:`--verbose` command line argument and the :envvar:`DTS_VERBOSE` environment
> +    variable control the verbosity of output. If enabled, all messages will be emitted to the
> +    console.
>   
> +    The :option:`--output` command line argument and the :envvar:`DTS_OUTPUT_DIR` environment
> +    variable modify the directory where the logs will be stored.
>   
> -class DTSLOG(logging.LoggerAdapter):
> -    """
> -    DTS log class for framework and testsuite.
> +    Attributes:
> +        node: The additional identifier. Currently unused.
> +        sh: The handler which emits logs to console.
> +        fh: The handler which emits logs to a file.
> +        verbose_fh: Just as fh, but logs with a different, more verbose, format.
>       """
>   
>       _logger: logging.Logger
> @@ -40,6 +42,15 @@ class DTSLOG(logging.LoggerAdapter):
>       verbose_fh: logging.FileHandler
>   
>       def __init__(self, logger: logging.Logger, node: str = "suite"):
> +        """Extend the constructor with additional handlers.
> +
> +        One handler logs to the console, the other one to a file, with either a regular or verbose
> +        format.
> +
> +        Args:
> +            logger: The logger from which to create the logger adapter.
> +            node: An additional identifier. Currently unused.
> +        """
>           self._logger = logger
>           # 1 means log everything, this will be used by file handlers if their level
>           # is not set
> @@ -92,26 +103,43 @@ def __init__(self, logger: logging.Logger, node: str = "suite"):
>           super(DTSLOG, self).__init__(self._logger, dict(node=self.node))
>   
>       def logger_exit(self) -> None:
> -        """
> -        Remove stream handler and logfile handler.
> -        """
> +        """Remove the stream handler and the logfile handler."""
>           for handler in (self.sh, self.fh, self.verbose_fh):
>               handler.flush()
>               self._logger.removeHandler(handler)
>   
>   
> +class _LoggerDictType(TypedDict):
> +    logger: DTSLOG
> +    name: str
> +    node: str
> +
> +
> +# List for saving all loggers in use
> +_Loggers: list[_LoggerDictType] = []
> +
> +
>   def getLogger(name: str, node: str = "suite") -> DTSLOG:
> +    """Get DTS logger adapter identified by name and node.
> +
> +    An existing logger will be return if one with the exact name and node already exists.

An existing logger will be return*ed*

> +    A new one will be created and stored otherwise.
> +
> +    Args:
> +        name: The name of the logger.
> +        node: An additional identifier for the logger.
> +
> +    Returns:
> +        A logger uniquely identified by both name and node.
>       """
> -    Get logger handler and if there's no handler for specified Node will create one.
> -    """
> -    global Loggers
> +    global _Loggers
>       # return saved logger
> -    logger: LoggerDictType
> -    for logger in Loggers:
> +    logger: _LoggerDictType
> +    for logger in _Loggers:
>           if logger["name"] == name and logger["node"] == node:
>               return logger["logger"]
>   
>       # return new logger
>       dts_logger: DTSLOG = DTSLOG(logging.getLogger(name), node)
> -    Loggers.append({"logger": dts_logger, "name": name, "node": node})
> +    _Loggers.append({"logger": dts_logger, "name": name, "node": node})
>       return dts_logger
> diff --git a/dts/framework/utils.py b/dts/framework/utils.py
> index f0c916471c..5016e3be10 100644
> --- a/dts/framework/utils.py
> +++ b/dts/framework/utils.py
> @@ -3,6 +3,16 @@
>   # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
>   # Copyright(c) 2022-2023 University of New Hampshire
>   
> +"""Various utility classes and functions.
> +
> +These are used in multiple modules across the framework. They're here because
> +they provide some non-specific functionality, greatly simplify imports or just don't
> +fit elsewhere.
> +
> +Attributes:
> +    REGEX_FOR_PCI_ADDRESS: The regex representing a PCI address, e.g. ``0000:00:08.0``.
> +"""
> +
>   import atexit
>   import json
>   import os
> @@ -19,12 +29,20 @@
>   
>   
>   def expand_range(range_str: str) -> list[int]:
> -    """
> -    Process range string into a list of integers. There are two possible formats:
> -    n - a single integer
> -    n-m - a range of integers
> +    """Process `range_str` into a list of integers.
> +
> +    There are two possible formats of `range_str`:
> +
> +        * ``n`` - a single integer,
> +        * ``n-m`` - a range of integers.
>   
> -    The returned range includes both n and m. Empty string returns an empty list.
> +    The returned range includes both ``n`` and ``m``. Empty string returns an empty list.
> +
> +    Args:
> +        range_str: The range to expand.
> +
> +    Returns:
> +        All the numbers from the range.
>       """
>       expanded_range: list[int] = []
>       if range_str:
> @@ -39,6 +57,14 @@ def expand_range(range_str: str) -> list[int]:
>   
>   
>   def get_packet_summaries(packets: list[Packet]) -> str:
> +    """Format a string summary from `packets`.
> +
> +    Args:
> +        packets: The packets to format.
> +
> +    Returns:
> +        The summary of `packets`.
> +    """
>       if len(packets) == 1:
>           packet_summaries = packets[0].summary()
>       else:
> @@ -49,6 +75,8 @@ def get_packet_summaries(packets: list[Packet]) -> str:
>   
>   
>   class StrEnum(Enum):
> +    """Enum with members stored as strings."""
> +
>       @staticmethod
>       def _generate_next_value_(
>           name: str, start: int, count: int, last_values: object
> @@ -56,22 +84,29 @@ def _generate_next_value_(
>           return name
>   
>       def __str__(self) -> str:
> +        """The string representation is the name of the member."""
>           return self.name
>   
>   
>   class MesonArgs(object):
> -    """
> -    Aggregate the arguments needed to build DPDK:
> -    default_library: Default library type, Meson allows "shared", "static" and "both".
> -               Defaults to None, in which case the argument won't be used.
> -    Keyword arguments: The arguments found in meson_options.txt in root DPDK directory.
> -               Do not use -D with them, for example:
> -               meson_args = MesonArgs(enable_kmods=True).
> -    """
> +    """Aggregate the arguments needed to build DPDK."""
>   
>       _default_library: str
>   
>       def __init__(self, default_library: str | None = None, **dpdk_args: str | bool):
> +        """Initialize the meson arguments.
> +
> +        Args:
> +            default_library: The default library type, Meson supports ``shared``, ``static`` and
> +                ``both``. Defaults to :data:`None`, in which case the argument won't be used.
> +            dpdk_args: The arguments found in ``meson_options.txt`` in root DPDK directory.
> +                Do not use ``-D`` with them.
> +
> +        Example:
> +            ::
> +
> +                meson_args = MesonArgs(enable_kmods=True).
> +        """
>           self._default_library = (
>               f"--default-library={default_library}" if default_library else ""
>           )
> @@ -83,6 +118,7 @@ def __init__(self, default_library: str | None = None, **dpdk_args: str | bool):
>           )
>   
>       def __str__(self) -> str:
> +        """The actual args."""
>           return " ".join(f"{self._default_library} {self._dpdk_args}".split())
>   
>   
> @@ -104,24 +140,14 @@ class _TarCompressionFormat(StrEnum):
>   
>   
>   class DPDKGitTarball(object):
> -    """Create a compressed tarball of DPDK from the repository.
> -
> -    The DPDK version is specified with git object git_ref.
> -    The tarball will be compressed with _TarCompressionFormat,
> -    which must be supported by the DTS execution environment.
> -    The resulting tarball will be put into output_dir.
> +    """Compressed tarball of DPDK from the repository.
>   
> -    The class supports the os.PathLike protocol,
> +    The class supports the :class:`os.PathLike` protocol,
>       which is used to get the Path of the tarball::
>   
>           from pathlib import Path
>           tarball = DPDKGitTarball("HEAD", "output")
>           tarball_path = Path(tarball)
> -
> -    Arguments:
> -        git_ref: A git commit ID, tag ID or tree ID.
> -        output_dir: The directory where to put the resulting tarball.
> -        tar_compression_format: The compression format to use.
>       """
>   
>       _git_ref: str
> @@ -136,6 +162,17 @@ def __init__(
>           output_dir: str,
>           tar_compression_format: _TarCompressionFormat = _TarCompressionFormat.xz,
>       ):
> +        """Create the tarball during initialization.
> +
> +        The DPDK version is specified with `git_ref`. The tarball will be compressed with
> +        `tar_compression_format`, which must be supported by the DTS execution environment.
> +        The resulting tarball will be put into `output_dir`.
> +
> +        Args:
> +            git_ref: A git commit ID, tag ID or tree ID.
> +            output_dir: The directory where to put the resulting tarball.
> +            tar_compression_format: The compression format to use.
> +        """
>           self._git_ref = git_ref
>           self._tar_compression_format = tar_compression_format
>   
> @@ -204,4 +241,5 @@ def _delete_tarball(self) -> None:
>               os.remove(self._tarball_path)
>   
>       def __fspath__(self) -> str:
> +        """The os.PathLike protocol implementation."""
>           return str(self._tarball_path)