Re: [PATCH 1/6] dts: add parameters data structure

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

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.