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 E92E843E32; Wed, 10 Apr 2024 11:15:27 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 66476402C5; Wed, 10 Apr 2024 11:15:27 +0200 (CEST) Received: from mail-ej1-f52.google.com (mail-ej1-f52.google.com [209.85.218.52]) by mails.dpdk.org (Postfix) with ESMTP id 8F1054029E for ; Wed, 10 Apr 2024 11:15:24 +0200 (CEST) Received: by mail-ej1-f52.google.com with SMTP id a640c23a62f3a-a51969e780eso799186466b.3 for ; Wed, 10 Apr 2024 02:15:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1712740524; x=1713345324; 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=RzB+uZHrl7wLBcsXb67Dvf9ITd7WxPT7Z1TP/IpDBAo=; b=RuYp8LpfEaYYhZlZ1Oy+0RK0XgyDHoMPkUxpYvBGoz2VNoQi1VqG48sl8WENKEWnDE 5hZxZgsxQv47ls9NiLJ29rg2rkZPeCu9vtk2j0IRWp3GjqxM4sRqGmzIgmAJEN0W3bwB wRZUSCoekX3UL+1HPHgFMG5Pi3QaU83NJ1188Q7ilj73/Yj1vyQcGb96Su9ZxsHXUsNE zi05f3Ur9iBFc+7CrwfslI4ylygWodRs4olWxeCreQVC45Xm9xU1PFDZrmWy7z8zZ9rl hP3/7l+5Y6I96mptcbFdOIL79G263+vfJgCAwglYw0ws3fTw3dUphrRt+v1HYSA8l6hY UH5Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1712740524; x=1713345324; 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=RzB+uZHrl7wLBcsXb67Dvf9ITd7WxPT7Z1TP/IpDBAo=; b=bxn0tiY5Wvpatb5iebAYhsnqMLGn/P/ECnSlrYueuBvQfkjlK6dP7REuje2RT6AycW L9EKt+OTCiQa7fzhVUdJLYpHfXmOe6o/MDCvrJE227tIdmnapemrJJVvysgpG9Ii0FKZ eWh3/zZHOIviWtAVzo4g848PwmwavBd2bs3Dz+40KQ0VVdPAtHTtBsStJUiehuwtO+25 RCLThxmWaCOIIe0W7v3JwVZkU2247/DqqLE3d4vl6U0ry/qihISNVN3tAwOflqfPY4y9 vI63lVL/jpNr2hpTLPBdmutdCvMUPC+nAV7dhzg3XSxHApMo4hztDp/LouDGsrE/08Oa gL1A== X-Gm-Message-State: AOJu0Ywj94KA88xec18UolFNXf1BEd19SjQHVFtSj3bPNHYNRa13pAqn SxlLnoKLx87cRvWgSgffvrwLaVrT9wFyFFsb7BOeMXU2uy97eg/uFydNO1mrmX36lZfHM7GffFl rZILVEIG/2EumrJeAKVUwUK1au+Jj7qBnkE/qVg== X-Google-Smtp-Source: AGHT+IFC5KjoYfINcAKt4HPIzjzKqEu3S7Sf6obBk6hf8vukgaNV5386Y6FL5SRi3XImnV7ZCVs8103XO3gpFfiQzeg= X-Received: by 2002:a17:906:b181:b0:a4e:7a8f:2101 with SMTP id w1-20020a170906b18100b00a4e7a8f2101mr1613801ejy.2.1712740524018; Wed, 10 Apr 2024 02:15:24 -0700 (PDT) MIME-Version: 1.0 References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240326190422.577028-2-luca.vizzarro@arm.com> In-Reply-To: From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Date: Wed, 10 Apr 2024 11:15:12 +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 Tue, Apr 9, 2024 at 6:28=E2=80=AFPM Luca Vizzarro wrote: > > Thank you so much for your review Juraj! > You're welcome! > >> + > >> +def value_only(metadata: dict[str, Any] =3D {}) -> dict[str, Any]: > >> + """Injects the value of the attribute as-is without flag. Metadat= a 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 make > everything a static method of Params, but then having to do Params.short > 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 this 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. > > 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 just > > pass a function that does those two things (or calls two different > > functions). Also, I'd just rename the mixin the func or something like > > that. > > > > The default of an argument should not be mutable, here's a quick > > explanation: https://docs.python-guide.org/writing/gotchas/#mutable-def= ault-arguments > > > Indeed the reason for which I create dictionaries, as I am treating them > as read only. I wanted to avoid to bloat the code with lots of `is None` > 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. > > I don't really like the name. The positional arguments are applied to > > the value and that should be reflected in the name - I'd like to see > > something like convert in the name. > > What this does is effectively just add the mixins to dataclass.field > metadata, hence "field"_mixins. This is meant to represent a chain of > mixins, in my original code this appeared more often. Not so much in > this post, as I made more optimisations. Which takes me to the plural > bit, for testpmd specifically this plurality appears only when > decorating another class using `str_mixins`, see TestPmdRingNUMAConfig. > So for consistency I kept both to ingest a chain of "mixins", as maybe > it'll be needed in the future. > > Are you suggesting to just make the name as singular? But still take > multiple function pointers? Singular with one function, as that was what I saw being used. The one function could do multiple things (or call multiple other functions) if a need arises. The str_mixins could be used this way as well. I don't know which one is better, maybe keeping the plural is fine. > The term "mixin" specifically just means a > middleware that manipulates the output value when using __str__. Aren't all of the other functions mixins as well, at least in some sense? They change the option, not the value, but could still be thought of as mixins in some respect. > Here we > are just chaining them for reusability. Do you have any better name in mi= nd? > 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. > >> + 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 None, = no?