Re: [PATCH v5 1/3] dts: rework arguments framework

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

On Thu, May 30, 2024 at 8:43 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> On 30/05/2024 16:30, Juraj Linkeš wrote:
> > There is a difference in behavior when I pass no arguments and then I
> > either have or don't have an env var set:
> > ./main.py
> > usage: main.py [-h] [--config-file FILE_PATH] ...
> > ...
> >
> > -----------------------------
> > DTS_SKIP_SETUP=Y ./main.py
> > main.py: error: one of the arguments --tarball/--snapshot
> > --revision/--rev/--git-ref is required
> >
> > For help and usage, run the command with the --help flag.
> >
> > I think this is because we're adding the env vars as actions (the
> > second scenario thus has at least one) and the first scenario doesn't
> > have any actions, which leads to the different output. I don't know
> > whether we want to unify these, but it's a bit confusing, as the error
> > applies to both cases.
>
> The behaviour is just as expected. You get the same exact results if you
> were to run:
>    ./main.py -s
> which is equivalent to:
>    DTS_SKIP_SETUP= ./main.py
>
> The true equivalent to your example would be:
>    ./main.py -s Y
>

Oh, so this is expected. I'm fine with this, but even though it's
equivalent, it doesn't look that way (at first glance), especially
when used with export:
export DTS_SKIP_SETUP=Y
... # other commands
./main.py -s

But I don't think that's a problem, so let's leave it as is.

> The only anomaly is that no matter the value passed (true, false, y,
> n...) it's always going to be set because the action is `store_true`.
> If we care to interpret a possible value then we need to implement a new
> action to cater for this.
>

The only interpretation we want to do is whether the variable is set
or not and it's documented as such (the docs say "set to any value"),
so there's not much of a point to interpret it. And this way it
mirrors the cmdline args (it either is there or isn't).

> >> diff --git a/dts/framework/settings.py b/dts/framework/settings.py
> >> index 688e8679a7..b19f274f9d 100644
> >> --- a/dts/framework/settings.py
> >> +++ b/dts/framework/settings.py
> >
> >> @@ -109,104 +116,224 @@ class Settings:
> >>
> >>   SETTINGS: Settings = Settings()
> >>
> >> +P = ParamSpec("P")
> >>
> >> -def _get_parser() -> argparse.ArgumentParser:
> >> -    """Create the argument parser for DTS.
> >> +#: Attribute name representing the env variable name to augment :class:`~argparse.Action` with.
> >> +_ENV_VAR_NAME_ATTR = "env_var_name"
> >> +#: Attribute name representing the value origin to augment :class:`~argparse.Action` with.
> >> +_IS_FROM_ENV_ATTR = "is_from_env"
> >>
> >> -    Command line options take precedence over environment variables, which in turn take precedence
> >> -    over default values.
> >> +#: The prefix to be added to all of the environment variables.
> >> +ENV_PREFIX = "DTS_"
> >> +
> >> +
> >> +def is_action_in_args(action: Action) -> bool:
> >
> > I don't think there's any expectation that these functions will be
> > used outside this module, so I'd make them all private, in which case
> > the short docstring would be fine.
> >
> > The ParamSpec also maybe should be private. Same goes for ENV_PREFIX.
>
> Ack.
>
> > I'm also looking at the order of the various functions, classes and
> > variables in the module and it looks all over the place. Maybe we can
> > tidy it up a bit.
>
> Could you please elaborate on this? It is also important to define
> what's the ordering expectation. At the moment they are mostly in order
> of usage, except for the Settings which I purposefully left on top as
> they are easier to find and probably the most "needed" for a user. As
> for the env var helper functions, most of them are used rightaway, but I
> left a couple that are used later there so that they are all grouped
> together (and related).
>

I just wanted to make sure there's some logic to it and the way you
described it sounds good.
Also, I was looking at the file with all of the patches applied, so
that made it look more jumbled than it is. Without the two functions
added later (or with them in the right place), the order actually
looks fine.

> >> +class ArgumentParser(argparse.ArgumentParser):
> >
> > I'd rename this to DTSArgumentParser and maybe also make the classes
> > (this one and the formatter) private.
> >
>
> Ack.
> >> +    Instead of printing usage on every error, it prints instructions
> >> +    on how to do it.
> >
> > This sentence is confusing - how to do what?
>
> This could do without the sentence to be honest, I'll just remove it.
>

Ok.

<snip>

> >> -    parser.add_argument(
> >> +    add_argument_to_parser_with_env = augment_add_argument_with_env(parser.add_argument)
> >
> > I'm wondering whether we could modify the actual add_argument methods
> > of ArgumentParser and _MutuallyExclusiveGroup, either the class
> > methods or instance methods. Have you tried that? It could be easier
> > to understand.
>
> I tried this, but it becomes overly complicated because add_argument is
> implemented in _ActionsContainer which in turn is inherited by several
> classes, which we ultimately use. The easiest and clearest approach is
> just a wrapper.
>

Ok, let's go with the wrapper then.

> > Or, alternatively, we could do:
> >
> > action = parser.add_argument(
> >      "--config-file",
> >      default=SETTINGS.config_file_path,
> >      type=Path,
> >      help="The configuration file that describes the test cases, SUTs
> > and targets.",
> >      metavar="FILE_PATH",
> > )
> > add_env_var_to_action(action, env_var_name="CFG_FILE")
> >
> > This makes what we're trying to do a bit clearer, but requires two
> > calls instead of one, so maybe it's not better. I'm not sure.
>
> I can drop the HOF and use it as `_add_env_var_to_action` as a clearer
> replacement.
>

Let's try it.