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 DC72B43E32; Wed, 10 Apr 2024 11:46:48 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7C58D402D4; Wed, 10 Apr 2024 11:46:48 +0200 (CEST) Received: from mail-ej1-f46.google.com (mail-ej1-f46.google.com [209.85.218.46]) by mails.dpdk.org (Postfix) with ESMTP id 2FBAB402C5 for ; Wed, 10 Apr 2024 11:46:47 +0200 (CEST) Received: by mail-ej1-f46.google.com with SMTP id a640c23a62f3a-a450bedffdfso912226466b.3 for ; Wed, 10 Apr 2024 02:46:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1712742407; x=1713347207; darn=dpdk.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=nCiPhxAMMGKL16sX/fr1YrpobcTy7gIltcyVHmwlU2E=; b=uS7hBp0r779OGF7MHvl7pUH/urlkHxwsaA03UH5fLvTcnmbg0ntMtDhTIViNVxbFTK IEk+bKF2P7Y3wdoHRXe8xeY2V7gs3yI2rtW3fF0BPYMi82oQLvHAnSNA8mOECuv0u7wt JolgfBS9cBK5i4Aqb2KVSsBS+YVe78636oWtyiH9SRd1gLFvv8S3pxM/ozMwyb4Vu0kA P8VT8MWEubHQnFMRRHkqjdbUl9cCTG4sPLj/3PxTV+xhCSN8jGNAZHrq/fIruvM/Lzja KaNAWzPnMBaCmZXLoBDKRMxZY+WKtTdUuFhh5phVTOKfNNndOvT4fberyOK8SNglfacm Pgxw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712742407; x=1713347207; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=nCiPhxAMMGKL16sX/fr1YrpobcTy7gIltcyVHmwlU2E=; b=MBGJwCjLq0AKXgpPxLCKMv4UlikDZCTk+hHmzDwJDQcshtUSgvIVmuMKqwjZX0nql0 g5qoY44jBXD7Cvk+7ooHfPINOzEJFPpEPWLlW0IPMeYfrvJXtQAwWCDjVyTivRWzkMIl Q2K5QzYltkTrav//YpsqKRT/cKAZx+nnSAZ6VEUIgUxyw758DtsEKsB37kizCGPIuJsw ulbd+tJMiPAmeWq/GoJJEIU2r06hqt+rmE+dfU60j1hKEO8N/zP4+fFL66KsogHT6hZm r9AaUeETHXh05IPcCwZN4fe47VeHNI2Je+JMOfLpSk1zrK4vHxYp7WoVodo+SKwqBZrf /QBA== X-Gm-Message-State: AOJu0YwmpnU+6yNnhOibA4mmzifNhqdWORK3ZhasAvp8gY8VZHcaimib LFrAMItJnFxCuoMBfuWyBpUphDMlfn/1Q9eQ1VYMcWLzvhVZVk1UA18WY34Fe9UMUhhXfLtUmn2 /8/3dj/UJhYYNy9NONEZ7RrcWmrsmxTWVgw9XG1hU/9Zy7CrEC2g= X-Google-Smtp-Source: AGHT+IHKZt5aB8CJ5jamVAUUxw7LDgByVkmwFSS73bPhnZqDHksoxTFFcgTj7vgkeuzLpJKy0CAmo+25QME8TE4qQJ8= X-Received: by 2002:a17:907:33d3:b0:a4e:5540:7c0c with SMTP id zk19-20020a17090733d300b00a4e55407c0cmr998683ejb.70.1712742406693; Wed, 10 Apr 2024 02:46:46 -0700 (PDT) MIME-Version: 1.0 References: <20240122182611.1904974-1-luca.vizzarro@arm.com> <20240318171704.798634-1-luca.vizzarro@arm.com> <20240318171704.798634-2-luca.vizzarro@arm.com> <0383a9e5-f62e-4ebd-b24a-0c5a0192baad@arm.com> In-Reply-To: <0383a9e5-f62e-4ebd-b24a-0c5a0192baad@arm.com> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 10 Apr 2024 11:46:35 +0200 Message-ID: Subject: Re: [PATCH v2 1/3] dts: rework arguments framework To: Luca Vizzarro Cc: dev@dpdk.org, Paul Szczepanek , Jack Bond-Preston Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Tue, Apr 9, 2024 at 5:14=E2=80=AFPM Luca Vizzarro wrote: > > On 04/04/2024 10:25, Juraj Linke=C5=A1 wrote: > > Judging from the code, this patch seems like a convoluted way to implem= ent: > > 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. > > > > > > > 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 =3D=3D 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 th= e 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.