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
