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.
