From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; Wed, 10 Apr 2024 12:04:21 +0200 (CEST)
Received: by mail-ej1-f50.google.com with SMTP id
a640c23a62f3a-a51c8274403so451974766b.1
for <dev@dpdk.org>; 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>
<CAOb5WZYhMPoHmt7kJVmqkA3o=NX07=KYy6N8XnGqUtABaQ6ETQ@mail.gmail.com>
<da88777d-4356-4f4b-b8e6-aa9a9c157d3a@arm.com>
<CAOb5WZYVToWDeo4Qyf66gJYJmZEkC+A19qUeW43tqO+-MjmyTQ@mail.gmail.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?= <juraj.linkes@pantheon.tech>
Date: Wed, 10 Apr 2024 12:04:09 +0200
Message-ID: <CAOb5WZapSaHkA9Pe9Vi2rNGpjF34xg1AEXVBpJadp=AkY1ax-Q@mail.gmail.com>
Subject: Re: [PATCH 1/6] dts: add parameters data structure
To: Luca Vizzarro <Luca.Vizzarro@arm.com>
Cc: dev@dpdk.org, Jack Bond-Preston <jack.bond-preston@arm.com>,
Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
On Wed, Apr 10, 2024 at 11:51=E2=80=AFAM Luca Vizzarro <Luca.Vizzarro@arm.c=
om> 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?
> >
> > <snip>
>