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

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

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