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 8742543E33; Wed, 10 Apr 2024 12:04:23 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 145EB402C7; Wed, 10 Apr 2024 12:04:23 +0200 (CEST) Received: from mail-ej1-f50.google.com (mail-ej1-f50.google.com [209.85.218.50]) by mails.dpdk.org (Postfix) with ESMTP id 6808F402C5 for ; Wed, 10 Apr 2024 12:04:21 +0200 (CEST) Received: by mail-ej1-f50.google.com with SMTP id a640c23a62f3a-a51c8274403so451974766b.1 for ; Wed, 10 Apr 2024 03:04:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1712743461; x=1713348261; 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=a8v01OiyEO4wxGUyfdLVeWtaAHQHsnyABResmrt94oM=; b=TEjQH8Fc451AhwRgirKhbG2yNV11FZ+3SmbnM+GUuRHtO5RX1RhLw9jBm6HF9MEklW ThHAriyAseSpVvU7/39rmOyWk/hJdMpwUxkxAqQBvDYe5Ia3SMDcOFNSfN3iupqkCeFu nI7z0lkMaRUIgTTPQjhjql+Y0uohmYzSHbk4LM8Uea84zNvVeC3MQcq5OYIQ3AdLT2zt b+yydm0U9aasY1BxACJuI/xxxaF4OXhZT9rOa/9U4WXhakfeF7mb3glE/sBpzBRTiT4H 9tS79UF/3qnVF2IyCrU40Ea7VJ3J1v5sNV9PA8wtHYwkH2F8ZNWzPheMphw6qb4gtLbT d9+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712743461; x=1713348261; 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=a8v01OiyEO4wxGUyfdLVeWtaAHQHsnyABResmrt94oM=; b=NUXRTP2pzkFglhfPjLAgsyMRjSfGOhexE4TvOk48XRMNISLCMkT3Jlkn0vqUuSxHsk Y5s/d2WEBPjq3lXzkfrze5guCpPgqo1MnveEaA/52ZRfvhikQX2j2YXqnjb53wXUuN/+ VRe6RJ9AxUH75ELKWIVmvMXjNKo5D/SEyG9RiAR4vjFIRjuMmsprhhcAruqmR7ag05g5 8H7Eu80dGPThmv1Al+Xb49KjR2o/e2OIKcFjmq55bfhFLqKNrj/XC73Mjj/J/KyUHDGk Io9S2tQt7mwLQlz6aa73oF2JelqGwxTjgim8kvcerZJGaesSw64e5235onC8RWhBGimI GGDA== X-Gm-Message-State: AOJu0YzPWL801SJYqymFVSvCiByYEFhSPYeISnlmYstZXO6zhChg23kR j+51pU5RfaWd/bp0VtJnuRfRe4yvkoT9U6afEbJwZ0m0oMsw1Npgd8nhhnt03ZfuJRidK0aabe8 6S0/oYN/+AtFXsDKNkaMav2TjAPQ5pmeXZreLjNN+qWtp9VzMgaU= X-Google-Smtp-Source: AGHT+IFbPkEAus7ksiTPmFN/6bq6T2r5hQw3Wk4bb3Iy1MLw7LpZxca5PI4gScbS2cW7kdv08xjn87ghMZzb/5jTVYs= X-Received: by 2002:a17:906:a0c7:b0:a51:e5ac:7b7 with SMTP id bh7-20020a170906a0c700b00a51e5ac07b7mr1158798ejb.71.1712743460970; Wed, 10 Apr 2024 03:04:20 -0700 (PDT) MIME-Version: 1.0 References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-2-luca.vizzarro@arm.com> <74ae1709-8ab3-444a-8074-1f0699f7bba5@arm.com> In-Reply-To: <74ae1709-8ab3-444a-8074-1f0699f7bba5@arm.com> From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 10 Apr 2024 12:04:09 +0200 Message-ID: Subject: Re: [PATCH 1/6] dts: add parameters data structure To: Luca Vizzarro Cc: dev@dpdk.org, Jack Bond-Preston , Honnappa Nagarahalli 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 Wed, Apr 10, 2024 at 11:51=E2=80=AFAM Luca Vizzarro wrote: > > On 10/04/2024 10:15, Juraj Linke=C5=A1 wrote: > >>>> + > >>>> +def value_only(metadata: dict[str, Any] =3D {}) -> dict[str, Any]: > >>>> + """Injects the value of the attribute as-is without flag. Metad= ata modifier for :func:`dataclasses.field`.""" > >>>> + return {**metadata, META_VALUE_ONLY: True} > >>> > >>> These methods, on the other hand, are used outside this module, so it > >>> makes sense to have them outside Params. It could be better to have > >>> them inside as static methods, as they're closely related. Looking at > >>> how they're used, we should unite the imports: > >>> 1. In testpmd_shell, they're imported directly (from framework.params > >>> import long) > >>> 2. In sut_node, just the params module is imported (from framework > >>> import params and then accessed via it: metadata=3Dparams.short("l")) > >>> > >> Having a shorter version may look less verbose. I agree that we can ma= ke > >> everything a static method of Params, but then having to do Params.sho= rt > >> etc everytime will make it look more verbose. So what option do we > >> prefer? The functions do belong to the params module nonetheless, and > >> therefore are meant to be used in conjunction with the Params class. > >> > > > > When I first saw the code, I liked the usage in sut_node better, e.g.: > > prefix: str =3D field(metadata=3Dparams.long("file-prefix")). I think t= his > > is because it's obvious where the function comes from. I'd do the > > longer version because I think most people are just going to glance at > > the code when they want to know how to properly implement an argument > > so the explicit nature could help with understanding how it should be > > done. > > Ack. > > >>> If we move these to Params, we could import Params and use them > >>> similarly to 2. Not sure which one is better. > >>> > >> > >> > >>>> +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] =3D {}) -= > dict[str, Any]: > >>> > >>> Any reason why mixins are plural? I've only seen this used with one > >>> argument, do you anticipate we'd need to use more than one? We could > >>> make this singular and if we ever need to do two things, we could jus= t > >>> pass a function that does those two things (or calls two different > >>> functions). Also, I'd just rename the mixin the func or something lik= e > >>> that. > >>> > >>> The default of an argument should not be mutable, here's a quick > >>> explanation: https://docs.python-guide.org/writing/gotchas/#mutable-d= efault-arguments > >> > >> > >> Indeed the reason for which I create dictionaries, as I am treating th= em > >> as read only. I wanted to avoid to bloat the code with lots of `is Non= e` > >> checks. But we can sacrifice this optimization for better code. > >> > > > > This would be the only place where we'd do the check, as I don't think > > we need the metadata argument in any of the other functions - those > > seem to be mutually exclusive, but maybe they're not? In any case, > > we'd need to fix this, I don't think treating them as read-only avoids > > the problem. > > > > They are not mutually exclusive. But thinking of it we can spare every > problem with having to chain "metadata" by letting the user do it > through the use of the pipe operator. > Sounds good. > >> Here we > >> are just chaining them for reusability. Do you have any better name in= mind? > >> > > > > I don't know, so let's brainstorm a bit. Let's start with the usage: > > portmask: int | None =3D field(default=3DNone, metadata=3Dfield_mixins(= hex)) > > > > Here it's not clear at all why it's called field_mixins, at least > > compared to the other functions which are not called mixins. I guess > > the other functions are predefined option mixins whereas we're > > supplying our own value mixins here. I also noticed that there's a bit > > of an inconsistency with the naming. The basic functions (long etc.) > > don't have the "field_" prefix, but this one does. Maybe a better name > > would be custom_mixins? Or value_mixins? Or custom_value? Or maybe > > convert_value? I like the last one: > > portmask: int | None =3D field(default=3DNone, metadata=3Dconvert_value= (hex)) > > metadata=3Dparams.convert_value(_port_to_pci, > > metadata=3Dparams.multiple(params.short("a"))), # in sut_node > > > > I think this is easier to grasp. I'm thinking about whether we need to > > have mixin(s) in the name and I don't think it adds much. If I'm a > > developer, I'm looking at these functions and I stumble upon > > convert_value, what I'm thinking is "Nice, I can do some conversion on > > the values I pass, how do I do that?", then I look at the signature > > and find out that I expect, that is I need to pass a function (or > > multiple function if I want to). I guess this comes down to the > > function name (field_mixins) not conveying what it's doing, rather > > what you're passing to it. > > > > So my conclusion from this brainstorming is that a better name would > > be convert_value. :-) > > > > Also, unrelated, but the context is lost. Another thing I just noticed > > is in the docstring: > > The ``metadata`` keyword argument can be used to chain metadata > > modifiers together. > > > > We're missing the Args: section in all of the docstrings (where we > > could put the above). Also the Returns: section. > > Sure, we can do convert_value. I am honestly not too fussed about > naming, and your proposal makes more sense. Ok. I like to think a lot about these names because I think it would save a considerable amount of time for future developers. > And as above, we can spare > the whole metadata problem. Using your example: > > metadata=3Dparams.short("a") | params.multiple() > I see what you meant, this is awesome. Intuitive, understandable, concise and easy to use. > >>>> + return {**metadata, META_MIXINS: mixins} > >>> > >>> metadata | {META_MIXINS: mixins} also creates a new dictionary with > >>> values from both and I think that would be more readable (since it's > >>> mentioned in docs: > >>> https://docs.python.org/3/library/stdtypes.html#mapping-types-dict). > >> > >> If we were to use `None` as default to the arguments, then this would = no > >> longer be needed. But great shout, wasn't aware of this feature added = in > >> 3.9. > >> > > > > It wouldn't? We'd still have to merge the dicts when metadata is not No= ne, no? > > > > >