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