From: Luca Vizzarro <Luca.Vizzarro@arm.com>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
Cc: dev@dpdk.org, Paul Szczepanek <paul.szczepanek@arm.com>,
Jack Bond-Preston <jack.bond-preston@arm.com>
Subject: Re: [PATCH v2 1/3] dts: rework arguments framework
Date: Tue, 9 Apr 2024 16:14:20 +0100 [thread overview]
Message-ID: <0383a9e5-f62e-4ebd-b24a-0c5a0192baad@arm.com> (raw)
In-Reply-To: <CAOb5WZYkDsa=9-L=punX304mmTFZxy=pOn9_V0Wk4LruPvwDkw@mail.gmail.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
next prev parent reply other threads:[~2024-04-09 15:14 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-22 18:26 [PATCH 0/4] dts: error and usage improvements Luca Vizzarro
2024-01-22 18:26 ` [PATCH 1/4] dts: constrain DPDK source flag Luca Vizzarro
2024-01-29 11:47 ` Juraj Linkeš
2024-02-23 19:09 ` Luca Vizzarro
2024-03-01 10:22 ` Juraj Linkeš
2024-01-22 18:26 ` [PATCH 2/4] dts: customise argparse error message Luca Vizzarro
2024-01-29 13:04 ` Juraj Linkeš
2024-02-23 19:12 ` Luca Vizzarro
2024-02-26 9:09 ` Juraj Linkeš
2024-01-22 18:26 ` [PATCH 3/4] dts: show help when DTS is ran without args Luca Vizzarro
2024-01-22 18:26 ` [PATCH 4/4] dts: log stderr with failed remote commands Luca Vizzarro
2024-01-29 13:10 ` Juraj Linkeš
2024-02-23 19:19 ` Luca Vizzarro
2024-02-26 9:05 ` Juraj Linkeš
2024-03-18 17:17 ` [PATCH v2 0/3] dts: error and usage improvements Luca Vizzarro
2024-03-18 17:17 ` [PATCH v2 1/3] dts: rework arguments framework Luca Vizzarro
2024-04-04 9:25 ` Juraj Linkeš
2024-04-09 15:14 ` Luca Vizzarro [this message]
2024-04-10 9:46 ` Juraj Linkeš
2024-03-18 17:17 ` [PATCH v2 2/3] dts: constrain DPDK source argument Luca Vizzarro
2024-03-18 17:17 ` [PATCH v2 3/3] dts: store stderr in RemoteCommandExecutionError Luca Vizzarro
2024-05-14 11:44 ` [PATCH v3 0/3] error and usage improvements Luca Vizzarro
2024-05-14 11:44 ` [PATCH v3 1/3] dts: rework arguments framework Luca Vizzarro
2024-05-14 11:55 ` [PATCH v3] " Luca Vizzarro
2024-05-14 11:44 ` [PATCH v3 2/3] dts: constrain DPDK source argument Luca Vizzarro
2024-05-14 11:44 ` [PATCH v3 3/3] dts: store stderr in RemoteCommandExecutionError Luca Vizzarro
2024-05-14 12:04 ` [PATCH v4 0/3] error and usage improvements Luca Vizzarro
2024-05-14 12:05 ` [PATCH v4 1/3] dts: update mypy static checker Luca Vizzarro
2024-05-14 12:05 ` [PATCH v4 2/3] dts: clean up config types Luca Vizzarro
2024-05-14 12:05 ` [PATCH v4 3/3] dts: rework arguments framework Luca Vizzarro
2024-05-14 12:10 ` [PATCH v5 0/3] error and usage improvements Luca Vizzarro
2024-05-14 12:10 ` [PATCH v5 1/3] dts: rework arguments framework Luca Vizzarro
2024-05-14 12:10 ` [PATCH v5 2/3] dts: constrain DPDK source argument Luca Vizzarro
2024-05-14 12:10 ` [PATCH v5 3/3] dts: store stderr in RemoteCommandExecutionError Luca Vizzarro
2024-05-14 15:26 ` [PATCH v5 0/3] error and usage improvements Luca Vizzarro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0383a9e5-f62e-4ebd-b24a-0c5a0192baad@arm.com \
--to=luca.vizzarro@arm.com \
--cc=dev@dpdk.org \
--cc=jack.bond-preston@arm.com \
--cc=juraj.linkes@pantheon.tech \
--cc=paul.szczepanek@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).