From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id EA81A43E2B; Tue, 9 Apr 2024 17:14:25 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6D3B0402C6; Tue, 9 Apr 2024 17:14:25 +0200 (CEST) Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mails.dpdk.org (Postfix) with ESMTP id EFBD240150 for ; Tue, 9 Apr 2024 17:14:23 +0200 (CEST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 70AC1139F; Tue, 9 Apr 2024 08:14:53 -0700 (PDT) Received: from [10.1.38.52] (FVFG51LCQ05N.cambridge.arm.com [10.1.38.52]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 875153F6C4; Tue, 9 Apr 2024 08:14:22 -0700 (PDT) Message-ID: <0383a9e5-f62e-4ebd-b24a-0c5a0192baad@arm.com> Date: Tue, 9 Apr 2024 16:14:20 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/3] dts: rework arguments framework To: =?UTF-8?Q?Juraj_Linke=C5=A1?= Cc: dev@dpdk.org, Paul Szczepanek , Jack Bond-Preston References: <20240122182611.1904974-1-luca.vizzarro@arm.com> <20240318171704.798634-1-luca.vizzarro@arm.com> <20240318171704.798634-2-luca.vizzarro@arm.com> Content-Language: en-GB From: Luca Vizzarro In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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. > > > 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