Re: [RFC PATCH v4 4/4] dts: format docstrigs to google format

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

On 8/31/23 11:04, Juraj Linkeš wrote:
> WIP: only one module is reformatted to serve as a demonstration.
> 
> The google format is documented here [0].
> 
> [0]: https://google.github.io/styleguide/pyguide.html
> 
> Signed-off-by: Juraj Linkeš <juraj.linkes@pantheon.tech>
> Acked-by: Jeremy Spweock <jspweock@iol.unh.edu>
> ---
>   dts/framework/testbed_model/node.py | 171 +++++++++++++++++++---------
>   1 file changed, 118 insertions(+), 53 deletions(-)
> 
> diff --git a/dts/framework/testbed_model/node.py b/dts/framework/testbed_model/node.py
> index 23efa79c50..619743ebe7 100644
> --- a/dts/framework/testbed_model/node.py
> +++ b/dts/framework/testbed_model/node.py
> @@ -3,8 +3,13 @@
>   # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
>   # Copyright(c) 2022-2023 University of New Hampshire
>   
> -"""
> -A node is a generic host that DTS connects to and manages.
> +"""Common functionality for node management.
> +
> +There's a base class, Node, that's supposed to be extended by other classes

This comment and all the following ones are all something of a nitpick.
This sounds too passive. Why not having something simpler like:
The virtual class Node is meant to be extended by other classes
with functionality specific to that node type.

> +with functionality specific to that node type.
> +The only part that can be used standalone is the Node.skip_setup static method,
> +which is a decorator used to skip method execution if skip_setup is passed
> +by the user on the cmdline or in an env variable.

I'd extend env to the full word as this is meant to go in the documentation.

>   """
>   
>   from abc import ABC
> @@ -35,10 +40,26 @@
>   
>   
>   class Node(ABC):
> -    """
> -    Basic class for node management. This class implements methods that
> -    manage a node, such as information gathering (of CPU/PCI/NIC) and
> -    environment setup.
> +    """The base class for node management.
> +
> +    It shouldn't be instantiated, but rather extended.
> +    It implements common methods to manage any node:
> +
> +       * connection to the node
> +       * information gathering of CPU
> +       * hugepages setup
> +
> +    Arguments:
> +        node_config: The config from the input configuration file.
> +
> +    Attributes:
> +        main_session: The primary OS-agnostic remote session used
> +            to communicate with the node.
> +        config: The configuration used to create the node.
> +        name: The name of the node.
> +        lcores: The list of logical cores that DTS can use on the node.
> +            It's derived from logical cores present on the node and user configuration.
> +        ports: The ports of this node specified in user configuration.
>       """
>   
>       main_session: OSSession
> @@ -77,9 +98,14 @@ def _init_ports(self) -> None:
>               self.configure_port_state(port)
>   
>       def set_up_execution(self, execution_config: ExecutionConfiguration) -> None:
> -        """
> -        Perform the execution setup that will be done for each execution
> -        this node is part of.
> +        """Execution setup steps.
> +
> +        Configure hugepages and call self._set_up_execution where
> +        the rest of the configuration steps (if any) are implemented.
> +
> +        Args:
> +            execution_config: The execution configuration according to which
> +                the setup steps will be taken.
>           """
>           self._setup_hugepages()
>           self._set_up_execution(execution_config)
> @@ -88,58 +114,78 @@ def set_up_execution(self, execution_config: ExecutionConfiguration) -> None:
>               self.virtual_devices.append(VirtualDevice(vdev))
>   
>       def _set_up_execution(self, execution_config: ExecutionConfiguration) -> None:
> -        """
> -        This method exists to be optionally overwritten by derived classes and
> -        is not decorated so that the derived class doesn't have to use the decorator.
> +        """Optional additional execution setup steps for derived classes.
> +
> +        Derived classes should overwrite this
> +        if they want to add additional execution setup steps.

I'd probably use need or require instead of want (it's the dev that 
wants, not the class)

>           """
>   
>       def tear_down_execution(self) -> None:
> -        """
> -        Perform the execution teardown that will be done after each execution
> -        this node is part of concludes.
> +        """Execution teardown steps.
> +
> +        There are currently no common execution teardown steps
> +        common to all DTS node types.
>           """
>           self.virtual_devices = []
>           self._tear_down_execution()
>   
>       def _tear_down_execution(self) -> None:
> -        """
> -        This method exists to be optionally overwritten by derived classes and
> -        is not decorated so that the derived class doesn't have to use the decorator.
> +        """Optional additional execution teardown steps for derived classes.
> +
> +        Derived classes should overwrite this
> +        if they want to add additional execution teardown steps.
>           """
>   
>       def set_up_build_target(
>           self, build_target_config: BuildTargetConfiguration
>       ) -> None:
> -        """
> -        Perform the build target setup that will be done for each build target
> -        tested on this node.
> +        """Build target setup steps.
> +
> +        There are currently no common build target setup steps
> +        common to all DTS node types.
> +
> +        Args:
> +            build_target_config: The build target configuration according to which
> +                the setup steps will be taken.
>           """
>           self._set_up_build_target(build_target_config)
>   
>       def _set_up_build_target(
>           self, build_target_config: BuildTargetConfiguration
>       ) -> None:
> -        """
> -        This method exists to be optionally overwritten by derived classes and
> -        is not decorated so that the derived class doesn't have to use the decorator.
> +        """Optional additional build target setup steps for derived classes.
> +
> +        Derived classes should optionally overwrite this

Is there a reason for the "optionally" here when it's absent in all the 
other functions?

> +        if they want to add additional build target setup steps.
>           """
>   
>       def tear_down_build_target(self) -> None:
> -        """
> -        Perform the build target teardown that will be done after each build target
> -        tested on this node.
> +        """Build target teardown steps.
> +
> +        There are currently no common build target teardown steps
> +        common to all DTS node types.
>           """
>           self._tear_down_build_target()
>   
>       def _tear_down_build_target(self) -> None:
> -        """
> -        This method exists to be optionally overwritten by derived classes and
> -        is not decorated so that the derived class doesn't have to use the decorator.
> +        """Optional additional build target teardown steps for derived classes.
> +
> +        Derived classes should overwrite this
> +        if they want to add additional build target teardown steps.
>           """
>   
>       def create_session(self, name: str) -> OSSession:
> -        """
> -        Create and return a new OSSession tailored to the remote OS.
> +        """Create and return a new OS-agnostic remote session.
> +
> +        The returned session won't be used by the node creating it.
> +        The session must be used by the caller.
> +        Will be cleaned up automatically.

I had a double take reading this before I understood that the subject 
was the previously mentioned session. I'd recommend to either add "it" 
or "the session". Also, it will be cleaned automatically... when? When I 
create a new session? when the node is deleted?

> +
> +        Args:
> +            name: The name of the session.
> +
> +        Returns:
> +            A new OS-agnostic remote session.
>           """
>           session_name = f"{self.name} {name}"
>           connection = create_session(
> @@ -186,14 +232,24 @@ def filter_lcores(
>           filter_specifier: LogicalCoreCount | LogicalCoreList,
>           ascending: bool = True,
>       ) -> list[LogicalCore]:
> -        """
> -        Filter the LogicalCores found on the Node according to
> -        a LogicalCoreCount or a LogicalCoreList.
> +        """Filter the node's logical cores that DTS can use.
>   
> -        If ascending is True, use cores with the lowest numerical id first
> -        and continue in ascending order. If False, start with the highest
> -        id and continue in descending order. This ordering affects which
> -        sockets to consider first as well.
> +        Logical cores that DTS can use are ones that are present on the node,
> +        but filtered according to user config.
> +        The filter_specifier will filter cores from those logical cores.
> +
> +        Args:
> +            filter_specifier: Two different filters can be used, one that specifies
> +                the number of logical cores per core, cores per socket and
> +                the number of sockets,
> +                the other that specifies a logical core list.
> +            ascending: If True, use cores with the lowest numerical id first
> +                and continue in ascending order. If False, start with the highest
> +                id and continue in descending order. This ordering affects which
> +                sockets to consider first as well.
> +
> +        Returns:
> +            A list of logical cores.
>           """
>           self._logger.debug(f"Filtering {filter_specifier} from {self.lcores}.")
>           return lcore_filter(
> @@ -203,17 +259,14 @@ def filter_lcores(
>           ).filter()
>   
>       def _get_remote_cpus(self) -> None:
> -        """
> -        Scan CPUs in the remote OS and store a list of LogicalCores.
> -        """
> +        """Scan CPUs in the remote OS and store a list of LogicalCores."""
>           self._logger.info("Getting CPU information.")
>           self.lcores = self.main_session.get_remote_cpus(self.config.use_first_core)
>   
>       def _setup_hugepages(self):
> -        """
> -        Setup hugepages on the Node. Different architectures can supply different
> -        amounts of memory for hugepages and numa-based hugepage allocation may need
> -        to be considered.
> +        """Setup hugepages on the Node.
> +
> +        Configure the hugepages only if they're specified in user configuration.
>           """
>           if self.config.hugepages:
>               self.main_session.setup_hugepages(
> @@ -221,8 +274,11 @@ def _setup_hugepages(self):
>               )
>   
>       def configure_port_state(self, port: Port, enable: bool = True) -> None:
> -        """
> -        Enable/disable port.
> +        """Enable/disable port.
> +
> +        Args:
> +            port: The port to enable/disable.
> +            enable: True to enable, false to disable.
>           """
>           self.main_session.configure_port_state(port, enable)
>   
> @@ -232,15 +288,19 @@ def configure_port_ip_address(
>           port: Port,
>           delete: bool = False,
>       ) -> None:
> -        """
> -        Configure the IP address of a port on this node.
> +        """Add an IP address to a port on this node.
> +
> +        Args:
> +            address: The IP address with mask in CIDR format.
> +                Can be either IPv4 or IPv6.
> +            port: The port to which to add the address.
> +            delete: If True, will delete the address from the port
> +                instead of adding it.
>           """
>           self.main_session.configure_port_ip_address(address, port, delete)
>   
>       def close(self) -> None:
> -        """
> -        Close all connections and free other resources.
> -        """
> +        """Close all connections and free other resources."""
>           if self.main_session:
>               self.main_session.close()
>           for session in self._other_sessions:
> @@ -249,6 +309,11 @@ def close(self) -> None:
>   
>       @staticmethod
>       def skip_setup(func: Callable[..., Any]) -> Callable[..., Any]:
> +        """A decorator that skips the decorated function.
> +
> +        When used, the decorator executes an empty lambda function
> +        instead of the decorated function.
> +        """
>           if SETTINGS.skip_setup:
>               return lambda *args: None
>           else: