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

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

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.