From: Luca Vizzarro <Luca.Vizzarro@arm.com>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>
Cc: dev@dpdk.org, Jack Bond-Preston <jack.bond-preston@arm.com>,
Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
Subject: Re: [PATCH 1/6] dts: add parameters data structure
Date: Wed, 10 Apr 2024 10:51:32 +0100 [thread overview]
Message-ID: <74ae1709-8ab3-444a-8074-1f0699f7bba5@arm.com> (raw)
In-Reply-To: <CAOb5WZYVToWDeo4Qyf66gJYJmZEkC+A19qUeW43tqO+-MjmyTQ@mail.gmail.com>
On 10/04/2024 10:15, Juraj Linkeš wrote:
>>>> +
>>>> +def value_only(metadata: dict[str, Any] = {}) -> dict[str, Any]:
>>>> + """Injects the value of the attribute as-is without flag. Metadata 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=params.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 = field(metadata=params.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.
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] = {}) -> 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-default-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.
>
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.
>> 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 = field(default=None, metadata=field_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 = field(default=None, metadata=convert_value(hex))
> metadata=params.convert_value(_port_to_pci,
> metadata=params.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. And as above, we can spare
the whole metadata problem. Using your example:
metadata=params.short("a") | params.multiple()
>>>> + 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?
>
> <snip>
next prev parent reply other threads:[~2024-04-10 9:51 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-26 19:04 [PATCH 0/6] dts: add testpmd params and statefulness Luca Vizzarro
2024-03-26 19:04 ` [PATCH 1/6] dts: add parameters data structure Luca Vizzarro
2024-03-28 16:48 ` Jeremy Spewock
2024-04-09 15:52 ` Luca Vizzarro
2024-04-09 12:10 ` Juraj Linkeš
2024-04-09 16:28 ` Luca Vizzarro
2024-04-10 9:15 ` Juraj Linkeš
2024-04-10 9:51 ` Luca Vizzarro [this message]
2024-04-10 10:04 ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 2/6] dts: use Params for interactive shells Luca Vizzarro
2024-03-28 16:48 ` Jeremy Spewock
2024-04-09 14:56 ` Juraj Linkeš
2024-04-10 9:34 ` Luca Vizzarro
2024-04-10 9:58 ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 3/6] dts: add testpmd shell params Luca Vizzarro
2024-03-28 16:48 ` Jeremy Spewock
2024-04-09 16:37 ` Juraj Linkeš
2024-04-10 10:49 ` Luca Vizzarro
2024-04-10 13:17 ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 4/6] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-04-09 19:12 ` Juraj Linkeš
2024-04-10 10:53 ` Luca Vizzarro
2024-04-10 13:18 ` Juraj Linkeš
2024-04-26 18:06 ` Jeremy Spewock
2024-04-29 7:45 ` Juraj Linkeš
2024-03-26 19:04 ` [PATCH 5/6] dts: add statefulness to InteractiveShell Luca Vizzarro
2024-03-28 16:48 ` Jeremy Spewock
2024-04-10 6:53 ` Juraj Linkeš
2024-04-10 11:27 ` Luca Vizzarro
2024-04-10 13:35 ` Juraj Linkeš
2024-04-10 14:07 ` Luca Vizzarro
2024-04-12 12:33 ` Juraj Linkeš
2024-04-29 14:48 ` Jeremy Spewock
2024-03-26 19:04 ` [PATCH 6/6] dts: add statefulness to TestPmdShell Luca Vizzarro
2024-03-28 16:48 ` Jeremy Spewock
2024-04-10 7:41 ` Juraj Linkeš
2024-04-10 11:35 ` Luca Vizzarro
2024-04-11 10:30 ` Juraj Linkeš
2024-04-11 11:47 ` Luca Vizzarro
2024-04-11 12:13 ` Juraj Linkeš
2024-04-11 13:59 ` Luca Vizzarro
2024-04-26 18:06 ` Jeremy Spewock
2024-04-29 12:06 ` Juraj Linkeš
2024-04-10 7:50 ` Juraj Linkeš
2024-04-10 11:37 ` Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 0/8] dts: add testpmd params Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 1/8] dts: add params manipulation module Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 2/8] dts: use Params for interactive shells Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 3/8] dts: refactor EalParams Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 4/8] dts: remove module-wide imports Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 5/8] dts: add testpmd shell params Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 6/8] dts: use testpmd params for scatter test suite Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 7/8] dts: rework interactive shells Luca Vizzarro
2024-05-09 11:20 ` [PATCH v2 8/8] dts: use Unpack for type checking and hinting Luca Vizzarro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=74ae1709-8ab3-444a-8074-1f0699f7bba5@arm.com \
--to=luca.vizzarro@arm.com \
--cc=dev@dpdk.org \
--cc=honnappa.nagarahalli@arm.com \
--cc=jack.bond-preston@arm.com \
--cc=juraj.linkes@pantheon.tech \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).