[v2,1/3] dts: rework arguments framework

  The existing argument handling in the code relies on basic argparse
functionality and a custom argparse action to integrate environment
variables. This commit improves the current handling by augmenting
argparse through a custom implementation of the arguments.

This rework implements the following improvements:
- There are duplicate expressions scattered throughout the code. To
  improve readability and maintainability, these are refactored
  into list/dict comprehensions or factory functions.
- Arguments are augmented with their own class allowing for more
  flexibility, enabling manipulation while reducing redundancy.
- Arguments are grouped within a new class. This class would
  track the origin of each argument, ensuring consistent input
  handling and facilitating debugging.
- Instead of relying solely on argument flags, error messages now
  accurately reference environment variables when applicable, enhancing
  user experience. For instance:

    error: environment variable DTS_DPDK_TARBALL: Invalid file

- Knowing the number of environment variables and arguments set
  allow for a useful help page display when none are provided.
- A display of which environment variables have been detected and their
  corresponding values in the help page, aiding user awareness.

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
---
 doc/guides/tools/dts.rst  |  53 +++--
 dts/framework/settings.py | 393 ++++++++++++++++++++++++++++----------
 2 files changed, 324 insertions(+), 122 deletions(-)
  

On Mon, Mar 18, 2024 at 6:17 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> The existing argument handling in the code relies on basic argparse
> functionality and a custom argparse action to integrate environment
> variables. This commit improves the current handling by augmenting
> argparse through a custom implementation of the arguments.
>
> This rework implements the following improvements:
> - There are duplicate expressions scattered throughout the code. To
>   improve readability and maintainability, these are refactored
>   into list/dict comprehensions or factory functions.
> - Arguments are augmented with their own class allowing for more
>   flexibility, enabling manipulation while reducing redundancy.
> - Arguments are grouped within a new class. This class would
>   track the origin of each argument, ensuring consistent input
>   handling and facilitating debugging.
> - Instead of relying solely on argument flags, error messages now
>   accurately reference environment variables when applicable, enhancing
>   user experience. For instance:
>
>     error: environment variable DTS_DPDK_TARBALL: Invalid file
>
> - Knowing the number of environment variables and arguments set
>   allow for a useful help page display when none are provided.
> - A display of which environment variables have been detected and their
>   corresponding values in the help page, aiding user awareness.
>

Judging from the code, this patch seems like a convoluted way to implement:
1. An association between an Argument and the corresponding
environment variable,
2. A better way to add the env vars names to the help message of each
argument as well as adding the current value if set,
3. A better error message where we replace argument names with env var
names for args where env vars are used.

But maybe there's more.

In any case, isn't there a simpler way to implement the above? I
originally thought extending something in the argparse module (like
the add_argument methods in ArgumentParser and
_MutuallyExclusiveGroup), but that doesn't seem as simple as maybe
just augmenting the actions that are returned with the add_argument
methods (maybe we could subclass ArgumentParser and add some method
for this, such as add_dts_argument?), where we could just add the
env_var_name (and maybe arg_name if needed) and modify the help
message the same way we do now (modifying the self._action.help
attribute). This should also be enough for 3, but even if not, we
could store what we need in the subclass.

Also, what seems to be missing is the modification of actual values of
SETTING with the env var values (this could be done somewhere in the
add_dts_argument method).

But overall I like this approach as it gives us the knowledge of
whether an env var was used or not. I have some comments that
illustrate why I think the patch is a bit convoluted.

> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Paul Szczepanek <paul.szczepanek@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> ---
>  doc/guides/tools/dts.rst  |  53 +++--
>  dts/framework/settings.py | 393 ++++++++++++++++++++++++++++----------
>  2 files changed, 324 insertions(+), 122 deletions(-)
>
> diff --git a/doc/guides/tools/dts.rst b/doc/guides/tools/dts.rst
> index 47b218b2c6..6993443389 100644
> --- a/doc/guides/tools/dts.rst
> +++ b/doc/guides/tools/dts.rst
> @@ -215,30 +215,41 @@ DTS is run with ``main.py`` located in the ``dts`` directory after entering Poet
>  .. code-block:: console
>
>     (dts-py3.10) $ ./main.py --help
> -   usage: main.py [-h] [--config-file CONFIG_FILE] [--output-dir OUTPUT_DIR] [-t TIMEOUT] [-v] [-s] [--tarball TARBALL] [--compile-timeout COMPILE_TIMEOUT] [--test-suite TEST_SUITE [TEST_CASES ...]] [--re-run RE_RUN]
> +   usage: main.py [-h] [--config-file FILE_PATH] [--output-dir DIR_PATH] [-t SECONDS] [-v] [-s] [--tarball FILE_PATH]
> +                  [--compile-timeout SECONDS] [--test-suite TEST_SUITE [TEST_CASES ...]] [--re-run N_TIMES]
>
> -   Run DPDK test suites. All options may be specified with the environment variables provided in brackets. Command line arguments have higher priority.
> +   Run DPDK test suites. All options may be specified with the environment variables provided in brackets. Command
> +   line arguments have higher priority.
>
>     options:
> -   -h, --help            show this help message and exit
> -   --config-file CONFIG_FILE
> -                         [DTS_CFG_FILE] The configuration file that describes the test cases, SUTs and targets. (default: ./conf.yaml)
> -   --output-dir OUTPUT_DIR, --output OUTPUT_DIR
> -                         [DTS_OUTPUT_DIR] Output directory where DTS logs and results are saved. (default: output)
> -   -t TIMEOUT, --timeout TIMEOUT
> -                         [DTS_TIMEOUT] The default timeout for all DTS operations except for compiling DPDK. (default: 15)
> -   -v, --verbose         [DTS_VERBOSE] Specify to enable verbose output, logging all messages to the console. (default: False)
> -   -s, --skip-setup      [DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG nodes. (default: False)
> -   --tarball TARBALL, --snapshot TARBALL, --git-ref TARBALL
> -                         [DTS_DPDK_TARBALL] Path to DPDK source code tarball or a git commit ID, tag ID or tree ID to test. To test local changes, first commit them, then use the commit ID with this option. (default: dpdk.tar.xz)
> -   --compile-timeout COMPILE_TIMEOUT
> -                         [DTS_COMPILE_TIMEOUT] The timeout for compiling DPDK. (default: 1200)
> -   --test-suite TEST_SUITE [TEST_CASES ...]
> -                         [DTS_TEST_SUITES] A list containing a test suite with test cases. The first parameter is the test suite name, and the rest are test case names, which are optional. May be specified multiple times. To specify multiple test suites in the environment
> -                         variable, join the lists with a comma. Examples: --test-suite suite case case --test-suite suite case ... | DTS_TEST_SUITES='suite case case, suite case, ...' | --test-suite suite --test-suite suite case ... | DTS_TEST_SUITES='suite, suite case, ...'
> -                         (default: [])
> -   --re-run RE_RUN, --re_run RE_RUN
> -                         [DTS_RERUN] Re-run each test case the specified number of times if a test failure occurs. (default: 0)
> +     -h, --help            show this help message and exit
> +     --config-file FILE_PATH
> +                           [DTS_CFG_FILE] The configuration file that describes the test cases, SUTs and targets.
> +                           (default: conf.yaml)
> +     --output-dir DIR_PATH, --output DIR_PATH
> +                           [DTS_OUTPUT_DIR] Output directory where DTS logs and results are saved. (default: output)
> +     -t SECONDS, --timeout SECONDS
> +                           [DTS_TIMEOUT] The default timeout for all DTS operations except for compiling DPDK.
> +                           (default: 15)
> +     -v, --verbose         [DTS_VERBOSE] Specify to enable verbose output, logging all messages to the console.
> +                           (default: False)
> +     -s, --skip-setup      [DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG nodes. (default: False)
> +     --tarball FILE_PATH, --snapshot FILE_PATH, --git-ref FILE_PATH
> +                           [DTS_DPDK_TARBALL] Path to DPDK source code tarball or a git commit ID,tag ID or tree ID to
> +                           test. To test local changes, first commit them, then use the commit ID with this option.
> +                           (default: dpdk.tar.xz)
> +     --compile-timeout SECONDS
> +                           [DTS_COMPILE_TIMEOUT] The timeout for compiling DPDK. (default: 1200)
> +     --test-suite TEST_SUITE [TEST_CASES ...]
> +                           [DTS_TEST_SUITES] A list containing a test suite with test cases. The first parameter is
> +                           the test suite name, and the rest are test case names, which are optional. May be specified
> +                           multiple times. To specify multiple test suites in the environment variable, join the lists
> +                           with a comma. Examples: --test-suite SUITE1 CASE1 CASE2 --test-suite SUITE2 CASE1 ... |
> +                           DTS_TEST_SUITES='SUITE1 CASE1 CASE2, SUITE2 CASE1, ...' | --test-suite SUITE1 --test-suite SUITE2
> +                           CASE1 ... | DTS_TEST_SUITES='SUITE1, SUITE2 CASE1, ...' (default: [])
> +     --re-run N_TIMES, --re_run N_TIMES
> +                           [DTS_RERUN] Re-run each test case the specified number of times if a test failure occurs.
> +                           (default: 0)
>
>
>  The brackets contain the names of environment variables that set the same thing.
> diff --git a/dts/framework/settings.py b/dts/framework/settings.py
> index 688e8679a7..421a9cb15b 100644
> --- a/dts/framework/settings.py
> +++ b/dts/framework/settings.py
> @@ -2,6 +2,7 @@
>  # Copyright(c) 2010-2021 Intel Corporation
>  # Copyright(c) 2022-2023 PANTHEON.tech s.r.o.
>  # Copyright(c) 2022 University of New Hampshire
> +# Copyright(c) 2024 Arm Limited
>
>  """Environment variables and command line arguments parsing.
>
> @@ -62,6 +63,7 @@
>  The module provides one key module-level variable:
>
>  Attributes:
> +    ARGS: The module level variable storing the state of the DTS arguments.
>      SETTINGS: The module level variable storing framework-wide DTS settings.
>
>  Typical usage example::
> @@ -72,14 +74,30 @@
>
>  import argparse
>  import os
> +import sys
>  from dataclasses import dataclass, field
>  from pathlib import Path
> -from typing import Any
> +from typing import Any, Generator, NamedTuple
>
>  from .config import TestSuiteConfig
>  from .utils import DPDKGitTarball
>
>
> +#: The prefix to be added to all of the environment variables.
> +ENV_PREFIX = "DTS_"
> +
> +
> +DPDK_TARBALL_PATH_ARGUMENT_NAME = "dpdk_tarball_path"
> +CONFIG_FILE_ARGUMENT_NAME = "config_file"
> +OUTPUT_DIR_ARGUMENT_NAME = "output_dir"
> +TIMEOUT_ARGUMENT_NAME = "timeout"
> +VERBOSE_ARGUMENT_NAME = "verbose"
> +SKIP_SETUP_ARGUMENT_NAME = "skip_setup"
> +COMPILE_TIMEOUT_ARGUMENT_NAME = "compile_timeout"
> +TEST_SUITES_ARGUMENT_NAME = "test_suites"
> +RERUN_ARGUMENT_NAME = "re_run"
> +
> +
>  @dataclass(slots=True)
>  class Settings:
>      """Default framework-wide user settings.
> @@ -110,126 +128,296 @@ class Settings:
>  SETTINGS: Settings = Settings()
>
>
> -def _get_parser() -> argparse.ArgumentParser:
> -    """Create the argument parser for DTS.
> +class ArgumentEnvPair(NamedTuple):
> +    """A named tuple pairing the argument identifiers with its environment variable."""
>
> -    Command line options take precedence over environment variables, which in turn take precedence
> -    over default values.
> +    #: The argument name.
> +    arg_name: str
>
> -    Returns:
> -        argparse.ArgumentParser: The configured argument parser with defined options.
> -    """
> +    #: The argument's associated environment variable name.
> +    env_var_name: str
> +
> +    #: The argument's associated :class:`argparse.Action` name.
> +    action_name: str | None
> +
> +
> +@dataclass
> +class Argument:
> +    """A class representing a DTS argument."""
> +
> +    #: The identifying name of the argument.
> +    #: It also translates into the corresponding :class:`Settings` attribute.
> +    name: str
> +
> +    #: A list of flags to pass to :meth:`argparse.ArgumentParser.add_argument`.
> +    flags: tuple[str, ...]
> +
> +    #: Any other keyword arguments to pass to :meth:`argparse.ArgumentParser.add_argument`.
> +    kwargs: dict[str, Any]
> +
> +    #: The corresponding environment variable name.
> +    #: It is prefixed with the value stored in `ENV_PREFIX`.
> +    #: If not specified, it is automatically generated from the :attr:`~name`.
> +    env_var_name: str
> +
> +    _from_env: bool = False
> +
> +    #: A reference to its corresponding :class:`argparse.Action`.
> +    _action: argparse.Action | None = None
> +
> +    def __init__(self, name: str, *flags: str, env_var_name: str | None = None, **kwargs: Any):
> +        """Class constructor.
> +
> +        If the `help` argument is passed, this is prefixed with the
> +        argument's corresponding environment variable in square brackets."""
> +
> +        self.name = name
> +        self.flags = flags
> +        self.kwargs = kwargs
> +        self.env_var_name = self._make_env_var(env_var_name)
> +
> +        if "help" in self.kwargs:
> +            self.kwargs["help"] = f"[{self.env_var_name}] {self.kwargs['help']}"
> +
> +    def add_to(self, parser: argparse._ActionsContainer):
> +        """Adds this argument to an :class:`argparse.ArgumentParser` instance."""
> +        self._action = parser.add_argument(*self.flags, dest=self.name, **self.kwargs)
> +
> +    def _make_env_var(self, env_var_name: str | None) -> str:
> +        """Make the environment variable name."""
> +        return f"{ENV_PREFIX}{env_var_name or self.name.upper()}"
> +
> +    def get_env_var(self) -> str | None:
> +        """Get environment variable if it was supplied instead of a command line flag."""
> +
> +        env_var_value = os.environ.get(self.env_var_name)
>
> -    def env_arg(env_var: str, default: Any) -> Any:
> -        """A helper function augmenting the argparse with environment variables.
> +        if env_var_value:
> +            # check if the user has supplied any of this argument's flags in the command line
> +            for flag in self.flags:
> +                if flag in sys.argv:
> +                    return None
>
> -        If the supplied environment variable is defined, then the default value
> -        of the argument is modified. This satisfies the priority order of
> -        command line argument > environment variable > default value.
> +        return env_var_value
>
> -        Args:
> -            env_var: Environment variable name.
> -            default: Default value.
> +    @property
> +    def from_env(self) -> bool:
> +        """Indicates if the argument value originated from the environment."""
> +        return self._from_env
>
> -        Returns:
> -            Environment variable or default value.
> +    def inject_env_var(self, env_value: str) -> ArgumentEnvPair:
> +        """Injects the environment variable as a program argument.
> +
> +        Injects this argument's flag with the supplied environment variable's value and
> +        returns an :class:`ArgumentEnvPair` object pairing this argument to its environment
> +        variable and :class:`argparse.Action`.
> +
> +        The help notice of the argument is updated to display that the environment variable
> +        has been correctly picked up by showing its recorded value.
> +
> +        .. note:: This method **must** be called after the argument has been added to the parser.
>          """
> -        return os.environ.get(env_var) or default
>
> -    parser = argparse.ArgumentParser(
> -        description="Run DPDK test suites. All options may be specified with the environment "
> -        "variables provided in brackets. Command line arguments have higher priority.",
> -        formatter_class=argparse.ArgumentDefaultsHelpFormatter,
> -    )
> +        assert self._action is not None
> +
> +        sys.argv[1:0] = [self.flags[0], env_value]
> +
> +        self._from_env = True
> +
> +        if "help" in self.kwargs:
> +            self.kwargs["help"] = f"{self.kwargs['help']} (env value: {env_value})"
> +        else:
> +            self.kwargs["help"] = f"(env value: {env_value})"
> +
> +        self._action.help = self.kwargs["help"]
> +
> +        return ArgumentEnvPair(
> +            arg_name=self.name,
> +            env_var_name=self.env_var_name,
> +            action_name=argparse._get_action_name(self._action),
> +        )
> +
> +
> +@dataclass
> +class ArgumentGroup:
> +    """A class grouping all the instances of :class:`Argument`.
> +
> +    This class provides indexing to access an :class:`Argument` by name:
> +
> +    >>> args["dpdk_revision_id"].env_var_name
> +    DTS_DPDK_REVISION_ID
> +
> +    And can be iterated to access all the arguments:
> +
> +    >>> arg_env_vars = [arg.env_var_name for arg in args]
> +    ['DPDK_TARBALL', ..]
> +    """
> +
> +    #: The arguments values as parsed by :class:`argparse.ArgumentParse`.
> +    values: argparse.Namespace
> +
> +    #: A dictionary pairing argument names to :class:`Argument` instances.
> +    _args: dict[str, Argument]
> +
> +    #: A list of :class:`ArgumentEnvPair` containing all the successfully injected environment variables.
> +    _env_vars: list[ArgumentEnvPair]
> +
> +    def __init__(self, *args: Argument):
> +        self._args = {arg.name: arg for arg in args}
> +        self._env_vars = []
> +
> +    def __getitem__(self, arg_name: str) -> Argument:
> +        return self._args.__getitem__(arg_name)

Looking at this, this class could've been just a subclassed dict. We
could set the attributes with setattr in __init__(). But at that
point, it looks to be the same as the namespace returned by
parser.parse_args(), apart from the environment_fed_arguments property
(more below), which we could do without.

>
> -    parser.add_argument(
> +    def __iter__(self) -> Generator[Argument, None, None]:
> +        yield from self._args.values()
> +
> +    def add_environment_fed_argument(self, env_pair: ArgumentEnvPair):
> +        """Add an injected environment variable."""
> +        self._env_vars.append(env_pair)
> +

We're already storing the arguments in the class, so we could just add
whatever is in ArgumentEnvPair to the argument and we have the
correspondence (looking at the Argument class again, we already have
that). The pair class seems redundant.

> +    @property
> +    def environment_fed_arguments(self) -> list[ArgumentEnvPair]:
> +        """Returns the list of all the successfully injected environment variables."""
> +        return self._env_vars
> +

And then we could get all of this from the stored arguments. Could be
just a tuple of (var_name, arg_name) of args with from_env == True.

> +
> +ARGS = ArgumentGroup(
> +    Argument(
> +        CONFIG_FILE_ARGUMENT_NAME,
>          "--config-file",
> -        default=env_arg("DTS_CFG_FILE", SETTINGS.config_file_path),
> +        default=SETTINGS.config_file_path,
>          type=Path,
> -        help="[DTS_CFG_FILE] The configuration file that describes the test cases, "
> -        "SUTs and targets.",
> -    )
> -
> -    parser.add_argument(
> +        help="The configuration file that describes the test cases, SUTs and targets.",
> +        metavar="FILE_PATH",
> +        env_var_name="CFG_FILE",
> +    ),
> +    Argument(
> +        OUTPUT_DIR_ARGUMENT_NAME,
>          "--output-dir",
>          "--output",
> -        default=env_arg("DTS_OUTPUT_DIR", SETTINGS.output_dir),
> -        help="[DTS_OUTPUT_DIR] Output directory where DTS logs and results are saved.",
> -    )
> -
> -    parser.add_argument(
> +        default=SETTINGS.output_dir,
> +        help="Output directory where DTS logs and results are saved.",
> +        metavar="DIR_PATH",
> +    ),
> +    Argument(
> +        TIMEOUT_ARGUMENT_NAME,
>          "-t",
>          "--timeout",
> -        default=env_arg("DTS_TIMEOUT", SETTINGS.timeout),
> +        default=SETTINGS.timeout,
>          type=float,
> -        help="[DTS_TIMEOUT] The default timeout for all DTS operations except for compiling DPDK.",
> -    )
> -
> -    parser.add_argument(
> +        help="The default timeout for all DTS operations except for compiling DPDK.",
> +        metavar="SECONDS",
> +    ),
> +    Argument(
> +        VERBOSE_ARGUMENT_NAME,
>          "-v",
>          "--verbose",
>          action="store_true",
> -        default=env_arg("DTS_VERBOSE", SETTINGS.verbose),
> -        help="[DTS_VERBOSE] Specify to enable verbose output, logging all messages "
> -        "to the console.",
> -    )
> -
> -    parser.add_argument(
> +        help="Specify to enable verbose output, logging all messages " "to the console.",
> +    ),
> +    Argument(
> +        SKIP_SETUP_ARGUMENT_NAME,
>          "-s",
>          "--skip-setup",
>          action="store_true",
> -        default=env_arg("DTS_SKIP_SETUP", SETTINGS.skip_setup),
> -        help="[DTS_SKIP_SETUP] Specify to skip all setup steps on SUT and TG nodes.",
> -    )
> -
> -    parser.add_argument(
> +        help="Specify to skip all setup steps on SUT and TG nodes.",
> +    ),
> +    Argument(
> +        DPDK_TARBALL_PATH_ARGUMENT_NAME,
>          "--tarball",
>          "--snapshot",
>          "--git-ref",
> -        default=env_arg("DTS_DPDK_TARBALL", SETTINGS.dpdk_tarball_path),
>          type=Path,
> -        help="[DTS_DPDK_TARBALL] Path to DPDK source code tarball or a git commit ID, "
> +        default=SETTINGS.dpdk_tarball_path,
> +        help="Path to DPDK source code tarball or a git commit ID,"
>          "tag ID or tree ID to test. To test local changes, first commit them, "
>          "then use the commit ID with this option.",
> -    )
> -
> -    parser.add_argument(
> +        metavar="FILE_PATH",
> +        env_var_name="DPDK_TARBALL",
> +    ),
> +    Argument(
> +        COMPILE_TIMEOUT_ARGUMENT_NAME,
>          "--compile-timeout",
> -        default=env_arg("DTS_COMPILE_TIMEOUT", SETTINGS.compile_timeout),
> +        default=SETTINGS.compile_timeout,
>          type=float,
> -        help="[DTS_COMPILE_TIMEOUT] The timeout for compiling DPDK.",
> -    )
> -
> -    parser.add_argument(
> +        help="The timeout for compiling DPDK.",
> +        metavar="SECONDS",
> +    ),
> +    Argument(
> +        TEST_SUITES_ARGUMENT_NAME,
>          "--test-suite",
>          action="append",
>          nargs="+",
> -        metavar=("TEST_SUITE", "TEST_CASES"),
> -        default=env_arg("DTS_TEST_SUITES", SETTINGS.test_suites),
> -        help="[DTS_TEST_SUITES] A list containing a test suite with test cases. "
> +        default=SETTINGS.test_suites,
> +        help="A list containing a test suite with test cases. "
>          "The first parameter is the test suite name, and the rest are test case names, "
>          "which are optional. May be specified multiple times. To specify multiple test suites in "
>          "the environment variable, join the lists with a comma. "
>          "Examples: "
> -        "--test-suite suite case case --test-suite suite case ... | "
> -        "DTS_TEST_SUITES='suite case case, suite case, ...' | "
> -        "--test-suite suite --test-suite suite case ... | "
> -        "DTS_TEST_SUITES='suite, suite case, ...'",
> -    )
> -
> -    parser.add_argument(
> +        "--test-suite SUITE1 CASE1 CASE2 --test-suite SUITE2 CASE1 ... | "
> +        "DTS_TEST_SUITES='SUITE1 CASE1 CASE2, SUITE2 CASE1, ...' | "
> +        "--test-suite SUITE1 --test-suite SUITE2 CASE1 ... | "
> +        "DTS_TEST_SUITES='SUITE1, SUITE2 CASE1, ...'",
> +        metavar=("TEST_SUITE", "TEST_CASES"),
> +    ),
> +    Argument(
> +        RERUN_ARGUMENT_NAME,
>          "--re-run",
>          "--re_run",
> -        default=env_arg("DTS_RERUN", SETTINGS.re_run),
> +        default=SETTINGS.re_run,
>          type=int,
> -        help="[DTS_RERUN] Re-run each test case the specified number of times "
> -        "if a test failure occurs.",
> +        help="Re-run each test case the specified number of times if a test failure occurs.",
> +        env_var_name="RERUN",
> +        metavar="N_TIMES",
> +    ),
> +)
> +
> +
> +class ArgumentParser(argparse.ArgumentParser):
> +    """ArgumentParser with a custom error message.
> +
> +    This custom version of ArgumentParser changes the error message to
> +    accurately reflect its origin if an environment variable is used
> +    as an argument.
> +
> +    Instead of printing usage on every error, it prints instructions
> +    on how to do it.
> +    """
> +
> +    def error(self, message):
> +        for _, env_var_name, action_name in ARGS.environment_fed_arguments:
> +            message = message.replace(
> +                f"argument {action_name}", f"environment variable {env_var_name}"
> +            )

I think this should also contain the env var value to be consistent
with the help message.

> +
> +        print(f"{self.prog}: error: {message}\n", file=sys.stderr)
> +        self.exit(2, "For help and usage, " "run the command with the --help flag.\n")
> +
> +
> +def _get_parser() -> ArgumentParser:
> +    """Create the argument parser for DTS.
> +
> +    Command line options take precedence over environment variables, which in turn take precedence
> +    over default values.
> +
> +    Returns:
> +        ArgumentParser: The configured argument parser with defined options.
> +    """
> +
> +    parser = ArgumentParser(
> +        description="Run DPDK test suites. All options may be specified with the environment "
> +        "variables provided in brackets. Command line arguments have higher priority.",
> +        formatter_class=argparse.ArgumentDefaultsHelpFormatter,
> +        allow_abbrev=False,
>      )
> +    for arg in ARGS:
> +        arg.add_to(parser)
>
>      return parser
>
>
> -def _process_test_suites(args: str | list[list[str]]) -> list[TestSuiteConfig]:
> +def _process_test_suites(args: list[list[str]]) -> list[TestSuiteConfig]:
>      """Process the given argument to a list of :class:`TestSuiteConfig` to execute.
>
>      Args:
> @@ -240,17 +428,11 @@ def _process_test_suites(args: str | list[list[str]]) -> list[TestSuiteConfig]:
>      Returns:
>          A list of test suite configurations to execute.
>      """
> -    if isinstance(args, str):
> -        # Environment variable in the form of "suite case case, suite case, suite, ..."
> -        args = [suite_with_cases.split() for suite_with_cases in args.split(",")]
> -
> -    test_suites_to_run = []
> -    for suite_with_cases in args:
> -        test_suites_to_run.append(
> -            TestSuiteConfig(test_suite=suite_with_cases[0], test_cases=suite_with_cases[1:])
> -        )
> +    if ARGS[TEST_SUITES_ARGUMENT_NAME].from_env:
> +        # Environment variable in the form of "SUITE1 CASE1 CASE2, SUITE2 CASE1, SUITE3, ..."
> +        args = [suite_with_cases.split() for suite_with_cases in args[0][0].split(",")]
>
> -    return test_suites_to_run
> +    return [TestSuiteConfig(test_suite, test_cases) for [test_suite, *test_cases] in args]
>
>
>  def get_settings() -> Settings:
> @@ -261,19 +443,28 @@ def get_settings() -> Settings:
>      Returns:
>          The new settings object.
>      """
> -    parsed_args = _get_parser().parse_args()
> -    return Settings(
> -        config_file_path=parsed_args.config_file,
> -        output_dir=parsed_args.output_dir,
> -        timeout=parsed_args.timeout,
> -        verbose=parsed_args.verbose,
> -        skip_setup=parsed_args.skip_setup,
> -        dpdk_tarball_path=Path(
> -            Path(DPDKGitTarball(parsed_args.tarball, parsed_args.output_dir))
> -            if not os.path.exists(parsed_args.tarball)
> -            else Path(parsed_args.tarball)
> -        ),
> -        compile_timeout=parsed_args.compile_timeout,
> -        test_suites=_process_test_suites(parsed_args.test_suite),
> -        re_run=parsed_args.re_run,
> +
> +    parser = _get_parser()
> +
> +    for arg in ARGS:
> +        env_value = arg.get_env_var()
> +        if env_value:
> +            env_pair = arg.inject_env_var(env_value)
> +            ARGS.add_environment_fed_argument(env_pair)

We're going through all of the args here so we could just do this when
creating the argument. I guess we'd need to modify the top-level error
message afterwards with the current class structure, but I'm sure even
that could be done when creating the argument with some refactoring.

> +
> +    if len(sys.argv) == 1:
> +        parser.print_help()
> +        sys.exit(1)
> +
> +    ARGS.values = parser.parse_args()
> +

The values attribute seems superfluous as we're only using it locally.

> +    ARGS.values.dpdk_tarball_path = Path(
> +        Path(DPDKGitTarball(ARGS.values.dpdk_tarball_path, ARGS.values.output_dir))
> +        if not os.path.exists(ARGS.values.dpdk_tarball_path)
> +        else Path(ARGS.values.dpdk_tarball_path)
>      )
> +
> +    ARGS.values.test_suites = _process_test_suites(ARGS.values.test_suites)
> +
> +    kwargs = {k: v for k, v in vars(ARGS.values).items() if hasattr(SETTINGS, k)}

This is here so that we don't use both arguments from the mutual
group, right? We could add a short comment explaining this.
Also, I think we don't need to use vars() here, the result should be the same.

> +    return Settings(**kwargs)
> --
> 2.34.1
>
  

On 04/04/2024 10:25, Juraj Linkeš wrote:
> Judging from the code, this patch seems like a convoluted way to implement:
> 1. An association between an Argument and the corresponding
> environment variable,
> 2. A better way to add the env vars names to the help message of each
> argument as well as adding the current value if set,
> 3. A better error message where we replace argument names with env var
> names for args where env vars are used.
> 
> But maybe there's more.
> 
> In any case, isn't there a simpler way to implement the above? I
> originally thought extending something in the argparse module (like
> the add_argument methods in ArgumentParser and
> _MutuallyExclusiveGroup), but that doesn't seem as simple as maybe
> just augmenting the actions that are returned with the add_argument
> methods (maybe we could subclass ArgumentParser and add some method
> for this, such as add_dts_argument?), where we could just add the
> env_var_name (and maybe arg_name if needed) and modify the help
> message the same way we do now (modifying the self._action.help
> attribute). This should also be enough for 3, but even if not, we
> could store what we need in the subclass.

I agree that the solutions appears somewhat convoluted, and I am indeed 
open to ideas on how to simplify the approach. I initially considered 
extending the argparse module, but as you said it's no particularly 
simple, and it wasn't written to be particularly extendable either. If
we want to go down this route, it would be somewhat hacky. I am not 
against it though. But, yeah, in retrospective some things can be easily 
integrated.

> Also, what seems to be missing is the modification of actual values of
> SETTING with the env var values (this could be done somewhere in the
> add_dts_argument method).

I don't think I am following what you mean by this. If you refer to 
updating the values of `SETTING` with the environment ones, this is done 
using `inject_env_variable` before the arguments are parsed. In a few 
words, that method just injects the environment arguments in sys.argv. 
Therefore to the ArgumentParser it just looks like they were supplied on 
the command line.

> But overall I like this approach as it gives us the knowledge of
> whether an env var was used or not. I have some comments that
> illustrate why I think the patch is a bit convoluted.
> 
<snip>
> 
> Looking at this, this class could've been just a subclassed dict. We
> could set the attributes with setattr in __init__(). But at that
> point, it looks to be the same as the namespace returned by
> parser.parse_args(), apart from the environment_fed_arguments property
> (more below), which we could do without.
> 

Yes, definitely. The main reason to store the namespaced values was in 
case this could turn out to be useful when debugging in the future. But 
if we think it's not worthwhile, it can reduce complexity.

> 
> We're already storing the arguments in the class, so we could just add
> whatever is in ArgumentEnvPair to the argument and we have the
> correspondence (looking at the Argument class again, we already have
> that). The pair class seems redundant.
>
> 
> And then we could get all of this from the stored arguments. Could be
> just a tuple of (var_name, arg_name) of args with from_env == True.
> 

And storing a pre-made list of environment-fed arguments. Yes, we could 
definitely walk through every argument as needed. Given the context and 
usage of this, I guess yeah, you are right in saying it's redundant. 
Happy to remove it.

> 
> I think this should also contain the env var value to be consistent
> with the help message.
> 

Ack.

> 
> We're going through all of the args here so we could just do this when
> creating the argument. I guess we'd need to modify the top-level error
> message afterwards with the current class structure, but I'm sure even
> that could be done when creating the argument with some refactoring.
> 

Yeah. I decided to do this separately to avoid duplicating the code for 
the mutual exclusion group (as per the next commit).

> 
> The values attribute seems superfluous as we're only using it locally.
> 

Ack.

> 
> This is here so that we don't use both arguments from the mutual
> group, right? We could add a short comment explaining this.
> Also, I think we don't need to use vars() here, the result should be the same.
> 

Yes, and any other argument we may want to add in the future that don't 
belong in SETTINGS. It's only selecting the arguments that already exist 
in SETTING and write values for that. Can add a comment.

Namespace doesn't appear to implement iteration. As per the doc page[1], 
the suggested way is to use vars to extract a dict, which we can iterate 
over.

[1] https://docs.python.org/3/library/argparse.html#argparse.Namespace
  

On Tue, Apr 9, 2024 at 5:14 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 04/04/2024 10:25, Juraj Linkeš wrote:
> > Judging from the code, this patch seems like a convoluted way to implement:
> > 1. An association between an Argument and the corresponding
> > environment variable,
> > 2. A better way to add the env vars names to the help message of each
> > argument as well as adding the current value if set,
> > 3. A better error message where we replace argument names with env var
> > names for args where env vars are used.
> >
> > But maybe there's more.
> >
> > In any case, isn't there a simpler way to implement the above? I
> > originally thought extending something in the argparse module (like
> > the add_argument methods in ArgumentParser and
> > _MutuallyExclusiveGroup), but that doesn't seem as simple as maybe
> > just augmenting the actions that are returned with the add_argument
> > methods (maybe we could subclass ArgumentParser and add some method
> > for this, such as add_dts_argument?), where we could just add the
> > env_var_name (and maybe arg_name if needed) and modify the help
> > message the same way we do now (modifying the self._action.help
> > attribute). This should also be enough for 3, but even if not, we
> > could store what we need in the subclass.
>
> I agree that the solutions appears somewhat convoluted, and I am indeed
> open to ideas on how to simplify the approach. I initially considered
> extending the argparse module, but as you said it's no particularly
> simple, and it wasn't written to be particularly extendable either. If
> we want to go down this route, it would be somewhat hacky. I am not
> against it though. But, yeah, in retrospective some things can be easily
> integrated.
>

Great, let's get a v2 and see where we end up.

> > Also, what seems to be missing is the modification of actual values of
> > SETTING with the env var values (this could be done somewhere in the
> > add_dts_argument method).
>
> I don't think I am following what you mean by this. If you refer to
> updating the values of `SETTING` with the environment ones, this is done
> using `inject_env_variable` before the arguments are parsed. In a few
> words, that method just injects the environment arguments in sys.argv.
> Therefore to the ArgumentParser it just looks like they were supplied on
> the command line.
>

My bad, I must've made a mistake when verifying this. It is working fine.

> > But overall I like this approach as it gives us the knowledge of
> > whether an env var was used or not. I have some comments that
> > illustrate why I think the patch is a bit convoluted.
> >
> <snip>
> >
> > Looking at this, this class could've been just a subclassed dict. We
> > could set the attributes with setattr in __init__(). But at that
> > point, it looks to be the same as the namespace returned by
> > parser.parse_args(), apart from the environment_fed_arguments property
> > (more below), which we could do without.
> >
>
> Yes, definitely. The main reason to store the namespaced values was in
> case this could turn out to be useful when debugging in the future. But
> if we think it's not worthwhile, it can reduce complexity.
>
> >
> > We're already storing the arguments in the class, so we could just add
> > whatever is in ArgumentEnvPair to the argument and we have the
> > correspondence (looking at the Argument class again, we already have
> > that). The pair class seems redundant.
> >
> >
> > And then we could get all of this from the stored arguments. Could be
> > just a tuple of (var_name, arg_name) of args with from_env == True.
> >
>
> And storing a pre-made list of environment-fed arguments. Yes, we could
> definitely walk through every argument as needed. Given the context and
> usage of this, I guess yeah, you are right in saying it's redundant.
> Happy to remove it.
>
> >
> > I think this should also contain the env var value to be consistent
> > with the help message.
> >
>
> Ack.
>
> >
> > We're going through all of the args here so we could just do this when
> > creating the argument. I guess we'd need to modify the top-level error
> > message afterwards with the current class structure, but I'm sure even
> > that could be done when creating the argument with some refactoring.
> >
>
> Yeah. I decided to do this separately to avoid duplicating the code for
> the mutual exclusion group (as per the next commit).
>
> >
> > The values attribute seems superfluous as we're only using it locally.
> >
>
> Ack.
>
> >
> > This is here so that we don't use both arguments from the mutual
> > group, right? We could add a short comment explaining this.
> > Also, I think we don't need to use vars() here, the result should be the same.
> >
>
> Yes, and any other argument we may want to add in the future that don't
> belong in SETTINGS. It's only selecting the arguments that already exist
> in SETTING and write values for that. Can add a comment.
>
> Namespace doesn't appear to implement iteration. As per the doc page[1],
> the suggested way is to use vars to extract a dict, which we can iterate
> over.
>
> [1] https://docs.python.org/3/library/argparse.html#argparse.Namespace

Ah, I get it now. Thanks for pointing this out.