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

["Juraj Linkeš" <juraj.linkes@pantheon.tech>]

On Tue, Oct 31, 2023 at 1:10 PM Yoan Picchi <yoan.picchi@foss.arm.com> wrote:
>
> 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.

I think they're pretty helpful.

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

Okay, makes sense.

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

I'll try to do this everywhere in docs.

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

Good point, that's a better wording.

> >           """
> >
> >       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?
>

I've actually caught this as well when addressing the previous
comment. I unified the wording (without optionally, it's reduntant).

> > +        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?
>

The session will be cleaned up as part of the node's cleanup. What
about this new wording:

The returned session won't be used by the node creating it. The
session must be used by
the caller. The session will be maintained for the entire lifecycle of
the node object,
at the end of which the session will be cleaned up automatically.

I've added a note:

Note:
    Any number of these supplementary sessions may be created.


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