[1/6] dts: add parameters data structure

  This commit introduces a new "params" module, which adds a new way
to manage command line parameters. The provided Params dataclass
is able to read the fields of its child class and produce a string
representation to supply to the command line. Any data structure
that is intended to represent command line parameters can inherit
it. The representation can then manipulated by using the dataclass
field metadata in conjunction with the provided functions:

* value_only, used to supply a value without forming an option/flag
* options_end, used to prefix with an options end delimiter (`--`)
* short, used to define a short parameter name, e.g. `-p`
* long, used to define a long parameter name, e.g. `--parameter`
* multiple, used to turn a list into repeating parameters
* field_mixins, used to manipulate the string representation of the
  value

Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
---
 dts/framework/params.py | 232 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 232 insertions(+)
 create mode 100644 dts/framework/params.py
  

Overall I like the idea of having a structured way of passing
command-line arguments to applications as strings and I think that
this is a well-abstracted approach. I also like that this approach
still supports the ability to pass strings "as-is" and use them as
parameters as well. That opens the door for potentially creating
dataclasses which only detail key-parameters that we assume you will
use, without blocking you from inputting whatever you want.

On Tue, Mar 26, 2024 at 3:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
<snip>
> +META_VALUE_ONLY = "value_only"
> +META_OPTIONS_END = "options_end"
> +META_SHORT_NAME = "short_name"
> +META_LONG_NAME = "long_name"
> +META_MULTIPLE = "multiple"
> +META_MIXINS = "mixins"
> +
> +

I might add some kind of block comment here as a separator that
delimits that metadata modifiers start here. Something like what is
written in scapy.py which creates sections for XML-RPC method vs ones
that are run by the docker container. This isn't something strictly
necessary, but it might help break things up and add a little more
explanation.

> +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}
> +
> +
<snip>

You could do the same thing here for mixins, but again, I'm not sure
it's really necessary.

> +def field_mixins(*mixins: Mixin, metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +    """Takes in a variable number of mixins to manipulate the value's rendering. Metadata modifier for :func:`dataclasses.field`.
> +
> +    The ``metadata`` keyword argument can be used to chain metadata modifiers together.
> +
> +    Mixins can be chained together, executed from right to left in the arguments list order.
> +
> +    Example:
> +
> +    .. code:: python
> +
> +        hex_bitmask: int | None = field(default=0b1101, metadata=field_mixins(hex, metadata=param_name("mask")))
> +
> +    will render as ``--mask=0xd``. The :func:`hex` built-in can be used as a mixin turning a valid integer into a hexadecimal representation.
> +    """
> +    return {**metadata, META_MIXINS: mixins}
<snip>
> 2.34.1
>
  

On Tue, Mar 26, 2024 at 8:04 PM Luca Vizzarro <luca.vizzarro@arm.com> wrote:
>
> This commit introduces a new "params" module, which adds a new way
> to manage command line parameters. The provided Params dataclass
> is able to read the fields of its child class and produce a string
> representation to supply to the command line. Any data structure
> that is intended to represent command line parameters can inherit
> it. The representation can then manipulated by using the dataclass
> field metadata in conjunction with the provided functions:
>
> * value_only, used to supply a value without forming an option/flag
> * options_end, used to prefix with an options end delimiter (`--`)
> * short, used to define a short parameter name, e.g. `-p`
> * long, used to define a long parameter name, e.g. `--parameter`
> * multiple, used to turn a list into repeating parameters
> * field_mixins, used to manipulate the string representation of the
>   value

We shouldn't list what the patch does, but rather explain the choices
made within. It seems to me the patch is here to give devs an API
instead of them having to construct strings - explaining why this
approach improves the old one should be in the commit message.

>
> Signed-off-by: Luca Vizzarro <luca.vizzarro@arm.com>
> Reviewed-by: Jack Bond-Preston <jack.bond-preston@arm.com>
> Reviewed-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> ---
>  dts/framework/params.py | 232 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 232 insertions(+)
>  create mode 100644 dts/framework/params.py
>
> diff --git a/dts/framework/params.py b/dts/framework/params.py
> new file mode 100644
> index 0000000000..6b48c8353e
> --- /dev/null
> +++ b/dts/framework/params.py
> @@ -0,0 +1,232 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2024 Arm Limited
> +
> +"""Parameter manipulation module.
> +
> +This module provides :class:`~Params` which can be used to model any data structure
> +that is meant to represent any command parameters.
> +"""
> +
> +from dataclasses import dataclass, field, fields
> +from typing import Any, Callable, Literal, Reversible, TypeVar, Iterable
> +from enum import Flag
> +
> +
> +T = TypeVar("T")
> +#: Type for a Mixin.
> +Mixin = Callable[[Any], str]
> +#: Type for an option parameter.
> +Option = Literal[True, None]
> +#: Type for a yes/no option parameter.
> +BooleanOption = Literal[True, False, None]
> +
> +META_VALUE_ONLY = "value_only"
> +META_OPTIONS_END = "options_end"
> +META_SHORT_NAME = "short_name"
> +META_LONG_NAME = "long_name"
> +META_MULTIPLE = "multiple"
> +META_MIXINS = "mixins"

These are only used in the Params class (but not set), so I'd either
move them there or make them private.

> +
> +
> +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"))

If we move these to Params, we could import Params and use them
similarly to 2. Not sure which one is better.

> +
> +
> +def short(name: str, metadata: dict[str, Any] = {}) -> dict[str, Any]:

It seems to me that having the metadata argument doesn't do anything
in these basic functions.

> +    """Overrides any parameter name with the given short option. Metadata modifier for :func:`dataclasses.field`.
> +
> +    .. code:: python
> +
> +        logical_cores: str | None = field(default="1-4", metadata=short("l"))
> +
> +    will render as ``-l=1-4`` instead of ``--logical-cores=1-4``.
> +    """
> +    return {**metadata, META_SHORT_NAME: name}
> +
> +
> +def long(name: str, metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +    """Overrides the inferred parameter name to the specified one. Metadata modifier for :func:`dataclasses.field`.
> +
> +    .. code:: python
> +
> +        x_name: str | None = field(default="y", metadata=long("x"))
> +
> +    will render as ``--x=y``, but the field is accessed and modified through ``x_name``.
> +    """
> +    return {**metadata, META_LONG_NAME: name}
> +
> +
> +def options_end(metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +    """Precedes the value with an options end delimiter (``--``). Metadata modifier for :func:`dataclasses.field`."""
> +    return {**metadata, META_OPTIONS_END: True}
> +
> +
> +def multiple(metadata: dict[str, Any] = {}) -> dict[str, Any]:
> +    """Specifies that this parameter is set multiple times. Must be a list. Metadata modifier for :func:`dataclasses.field`.
> +
> +    .. code:: python
> +
> +        ports: list[int] | None = field(default_factory=lambda: [0, 1, 2], metadata=multiple(param_name("port")))
> +
> +    will render as ``--port=0 --port=1 --port=2``. Note that modifiers can be chained like in this example.
> +    """
> +    return {**metadata, META_MULTIPLE: True}
> +
> +
> +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

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.

> +    """Takes in a variable number of mixins to manipulate the value's rendering. Metadata modifier for :func:`dataclasses.field`.
> +
> +    The ``metadata`` keyword argument can be used to chain metadata modifiers together.
> +
> +    Mixins can be chained together, executed from right to left in the arguments list order.
> +
> +    Example:
> +
> +    .. code:: python
> +
> +        hex_bitmask: int | None = field(default=0b1101, metadata=field_mixins(hex, metadata=param_name("mask")))
> +
> +    will render as ``--mask=0xd``. The :func:`hex` built-in can be used as a mixin turning a valid integer into a hexadecimal representation.
> +    """
> +    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).

> +
> +
> +def _reduce_mixins(mixins: Reversible[Mixin], value: Any) -> str:
> +    for mixin in reversed(mixins):
> +        value = mixin(value)
> +    return value
> +
> +
> +def str_mixins(*mixins: Mixin):

Again the name, this doesn't strike me as a decorator name. A
decorator modifies the thing it's decorating so it should contain a
verb describing what it's doing.

Maybe we could also do singular mixin here, as I described above. It
may result in cleaner API.

> +    """Decorator which modifies the ``__str__`` method, enabling support for mixins.
> +
> +    Mixins can be chained together, executed from right to left in the arguments list order.
> +
> +    Example:
> +
> +    .. code:: python
> +
> +        @str_mixins(hex_from_flag_value)
> +        class BitMask(enum.Flag):
> +            A = auto()
> +            B = auto()
> +
> +    will allow ``BitMask`` to render as a hexadecimal value.
> +    """
> +
> +    def _class_decorator(original_class):
> +        original_class.__str__ = lambda self: _reduce_mixins(mixins, self)
> +        return original_class
> +
> +    return _class_decorator
> +
> +
> +def comma_separated(values: Iterable[T]) -> str:
> +    """Mixin which renders an iterable in a comma-separated string."""
> +    return ",".join([str(value).strip() for value in values if value is not None])
> +
> +
> +def bracketed(value: str) -> str:
> +    """Mixin which adds round brackets to the input."""
> +    return f"({value})"
> +
> +
> +def str_from_flag_value(flag: Flag) -> str:
> +    """Mixin which returns the value from a :class:`enum.Flag` as a string."""
> +    return str(flag.value)
> +
> +
> +def hex_from_flag_value(flag: Flag) -> str:
> +    """Mixin which turns a :class:`enum.Flag` value into hexadecimal."""
> +    return hex(flag.value)
> +
> +
> +def _make_option(param_name: str, short: bool = False, no: bool = False) -> str:
> +    param_name = param_name.replace("_", "-")
> +    return f"{'-' if short else '--'}{'no-' if no else ''}{param_name}"
> +
> +
> +@dataclass
> +class Params:
> +    """Helper abstract dataclass that renders its fields into command line arguments.

Let's make the abstract class explicitly abstract with
https://docs.python.org/3/library/abc.html#abc.ABC. It won't be a full
abstract class since it won't have any abstract method or properties,
but I think it'll be better this way.

> +
> +    The parameter name is taken from the field name by default. The following:
> +
> +    .. code:: python
> +
> +        name: str | None = "value"
> +
> +    is rendered as ``--name=value``.
> +    Through :func:`dataclasses.field` the resulting parameter can be manipulated by applying
> +    appropriate metadata. This class can be used with the following metadata modifiers:
> +
> +    * :func:`value_only`
> +    * :func:`options_end`
> +    * :func:`short`
> +    * :func:`long`
> +    * :func:`multiple`
> +    * :func:`field_mixins`
> +
> +    To use fields as option switches set the value to ``True`` to enable them. If you

There should be a comma between switches and set.

> +    use a yes/no option switch you can also set ``False`` which would enable an option
> +    prefixed with ``--no-``. Examples:
> +
> +    .. code:: python
> +
> +        interactive: Option = True  # renders --interactive
> +        numa: BooleanOption = False # renders --no-numa

I'm wondering whether these could be simplified, but it's pretty good
this way. I'd change the names though:
Option -> Switch
BooleanOption -> NoSwitch (or YesNoSwitch? NegativeSwitch? Not sure
about this one)

All options (short, long, etc.) are options so that's where it's
confusing. The BooleanOption doesn't really capture what we're doing
with it (prepending no-) so I want a name that captures it.

There could also be some confusion with this being different from the
rest of the options API. This only requires us to set the type, the
rest must be passed in dataclasses.field. It's probably not that big
of a deal though.

> +
> +    Setting ``None`` will disable any option.

I'd reword this to "Setting an option to ``None`` will prevent it from
being rendered." or something similar. Disabling an option is kinda
ambiguous.

The :attr:`~Option` type alias is provided for
> +    regular option switches, whereas :attr:`~BooleanOption` is offered for yes/no ones.
> +
> +    An instance of a dataclass inheriting ``Params`` can also be assigned to an attribute, this helps with grouping parameters
> +    together. The attribute holding the dataclass will be ignored and the latter will just be rendered as expected.
> +    """
> +
> +    def __str__(self) -> str:
> +        arguments: list[str] = []
> +
> +        for field in fields(self):
> +            value = getattr(self, field.name)
> +
> +            if value is None:
> +                continue
> +
> +            options_end = field.metadata.get(META_OPTIONS_END, False)
> +            if options_end:
> +                arguments.append("--")

This is a confusing way to separate the options. The separator itself
is an option (I'd rename it to sep or separator instead of end), so
I'd make a separate field for it. We could pass init=False to field()
in the field definition, but that relies on fields being ordered, so
we'd need to also pass order=True to the dataclass constructor.

> +
> +            value_only = field.metadata.get(META_VALUE_ONLY, False)
> +            if isinstance(value, Params) or value_only or options_end:
> +                arguments.append(str(value))
> +                continue
> +
> +            # take "short_name" metadata, or "long_name" metadata, or infer from field name
> +            option_name = field.metadata.get(
> +                META_SHORT_NAME, field.metadata.get(META_LONG_NAME, field.name)
> +            )
> +            is_short = META_SHORT_NAME in field.metadata
> +
> +            if isinstance(value, bool):
> +                arguments.append(_make_option(option_name, short=is_short, no=(not value)))
> +                continue
> +
> +            option = _make_option(option_name, short=is_short)
> +            separator = " " if is_short else "="
> +            str_mixins = field.metadata.get(META_MIXINS, [])
> +            multiple = field.metadata.get(META_MULTIPLE, False)
> +
> +            values = value if multiple else [value]
> +            for entry_value in values:
> +                entry_value = _reduce_mixins(str_mixins, entry_value)
> +                arguments.append(f"{option}{separator}{entry_value}")
> +
> +        return " ".join(arguments)
> +
> +
> +@dataclass
> +class StrParams(Params):
> +    """A drop-in replacement for parameters passed as a string."""
> +
> +    value: str = field(metadata=value_only())
> --
> 2.34.1
>
  

Thank you for your review Jeremy!

On 28/03/2024 16:48, Jeremy Spewock wrote:
> I might add some kind of block comment here as a separator that
> delimits that metadata modifiers start here. Something like what is
> written in scapy.py which creates sections for XML-RPC method vs ones
> that are run by the docker container. This isn't something strictly
> necessary, but it might help break things up and add a little more
> explanation.
<snip>
> You could do the same thing here for mixins, but again, I'm not sure
> it's really necessary.

Yes, I agree that using block comments to delimit sections is a good 
idea. I wasn't sure if we had an established way of doing this, and 
looks like we do!
  

Thank you so much for your review Juraj!

On 09/04/2024 13:10, Juraj Linkeš wrote:
> We shouldn't list what the patch does, but rather explain the choices
> made within. It seems to me the patch is here to give devs an API
> instead of them having to construct strings - explaining why this
> approach improves the old one should be in the commit message.
>

Ack.
>> +META_VALUE_ONLY = "value_only"
>> +META_OPTIONS_END = "options_end"
>> +META_SHORT_NAME = "short_name"
>> +META_LONG_NAME = "long_name"
>> +META_MULTIPLE = "multiple"
>> +META_MIXINS = "mixins"
> 
> These are only used in the Params class (but not set), so I'd either
> move them there or make them private.
>

Ack.

>> +
>> +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.

> 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.

> 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? The term "mixin" specifically just means a 
middleware that manipulates the output value when using __str__. Here we 
are just chaining them for reusability. Do you have any better name in mind?

>> +    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.

>> +def _reduce_mixins(mixins: Reversible[Mixin], value: Any) -> str:
>> +    for mixin in reversed(mixins):
>> +        value = mixin(value)
>> +    return value
>> +
>> +
>> +def str_mixins(*mixins: Mixin):
> 
> Again the name, this doesn't strike me as a decorator name. A
> decorator modifies the thing it's decorating so it should contain a
> verb describing what it's doing.
> 
> Maybe we could also do singular mixin here, as I described above. It
> may result in cleaner API.

Great point. Yes making the function name sound like a verb is 
definitely the right way. Thank you for pointing it out.

>> +@dataclass
>> +class Params:
>> +    """Helper abstract dataclass that renders its fields into command line arguments.
> 
> Let's make the abstract class explicitly abstract with
> https://docs.python.org/3/library/abc.html#abc.ABC. It won't be a full
> abstract class since it won't have any abstract method or properties,
> but I think it'll be better this way.

No problem. I'm not sure if applying ABC to a dataclass may complicate 
things. But can definitely do.

>> +    To use fields as option switches set the value to ``True`` to enable them. If you
> 
> There should be a comma between switches and set.

Ack.

>> +    use a yes/no option switch you can also set ``False`` which would enable an option
>> +    prefixed with ``--no-``. Examples:
>> +
>> +    .. code:: python
>> +
>> +        interactive: Option = True  # renders --interactive
>> +        numa: BooleanOption = False # renders --no-numa
> 
> I'm wondering whether these could be simplified, but it's pretty good
> this way. I'd change the names though:
> Option -> Switch
> BooleanOption -> NoSwitch (or YesNoSwitch? NegativeSwitch? Not sure
> about this one)
> 
> All options (short, long, etc.) are options so that's where it's
> confusing. The BooleanOption doesn't really capture what we're doing
> with it (prepending no-) so I want a name that captures it.
> 
> There could also be some confusion with this being different from the
> rest of the options API. This only requires us to set the type, the
> rest must be passed in dataclasses.field. It's probably not that big
> of a deal though.
> 

I am glad you are bringing this up. I am also perplexed on the choice of 
name here. I decided to use whatever libc getopts uses. But your 
suggestion sounds nice. Will use it.

>> +
>> +    Setting ``None`` will disable any option.
> 
> I'd reword this to "Setting an option to ``None`` will prevent it from
> being rendered." or something similar. Disabling an option is kinda
> ambiguous.
> 

Ack.

>> +            options_end = field.metadata.get(META_OPTIONS_END, False)
>> +            if options_end:
>> +                arguments.append("--")
> 
> This is a confusing way to separate the options. The separator itself
> is an option (I'd rename it to sep or separator instead of end), so
> I'd make a separate field for it. We could pass init=False to field()
> in the field definition, but that relies on fields being ordered, so
> we'd need to also pass order=True to the dataclass constructor.

Ack.
  

On Tue, Apr 9, 2024 at 6:28 PM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> Thank you so much for your review Juraj!
>

You're welcome!

> >> +
> >> +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.

> > 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.

> > 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 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.

> >> +    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>
  

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>
  

On Wed, Apr 10, 2024 at 11:51 AM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:
>
> 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.
>

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 = 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.

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=params.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 None, no?
> >
> > <snip>
>