On Tue, Apr 9, 2024 at 5:14 PM Luca Vizzarro <luca.vizza...@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.
