From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id E7D2144172; Thu, 6 Jun 2024 11:19:36 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D3DA94027D; Thu, 6 Jun 2024 11:19:36 +0200 (CEST) Received: from mail-ed1-f48.google.com (mail-ed1-f48.google.com [209.85.208.48]) by mails.dpdk.org (Postfix) with ESMTP id C48E740268 for ; Thu, 6 Jun 2024 11:19:33 +0200 (CEST) Received: by mail-ed1-f48.google.com with SMTP id 4fb4d7f45d1cf-57a1fe639a5so751280a12.1 for ; Thu, 06 Jun 2024 02:19:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1717665573; x=1718270373; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=AN9FIFjvazF4O2fzmbsTpbqs9doZQKk+4vdtF5lhvIs=; b=rzlhpqbVHzAa7bsJe1lk7FHImhTry16GYyvcdSF4QSVdrp6+jFXzX47WxuCuutIw+c nWrcTQ+wfqD0pA2BfqUEp9N4sB9SX/0EkEy8c/aPtYcFCQN8HCuxiYTppdReofg4XqZb L52Scy0Mn7Al04dzhidX1SkgOqZT7Ict/sM8tRRsa/Feep994Rq+cfxKZFG88uoN5qdx kzfZzzF5l2P71Yg1Xwyp67QccSyQ0A6yqkBKyjZ3DzVcjzLPt7WTCVhol2B3njbHb3wG EeWFFC1KmONgMjLY34ggfxxtuaOiVMnJjtV2wbE/REBVDLIZr7mll7Fv+nSx/3HOlkq4 Sn2Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717665573; x=1718270373; h=content-transfer-encoding:in-reply-to:content-language:references :cc:to:subject:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=AN9FIFjvazF4O2fzmbsTpbqs9doZQKk+4vdtF5lhvIs=; b=LHcVmlnX8N2GBlWcvP/v3Cig8NgjrK1IOr0uwTHJitZzQYgHNO5ObbQ+wFmw4pGim0 n0rWzZ9G/aYpO7ExJ7+SPTg+M7hgrl0nw+UdSwhRzCTjP7Iocbbmw+qDxhh0VZzkUPXL 0rs+QFmKs1k4ECKm6vOLC2LH3r/X/pBk5gjdyMAzNf5Y8yccx95YjPAw1eqlcEPCWGgv NE0GWt5uX4FsnFrCYGT3Wbl8HV57GnH9rlM+bUkDCdb17PLdQiUhYxx7wIlLRS871rIO 6qEKyX/NK2t0wQ1/3ZJM3OyEuK6ORfVh87WsQJhWn2N3nHLlhcko0SQAb5ypuVWcFG6i Ogcw== X-Forwarded-Encrypted: i=1; AJvYcCXXnVPj8Tl4JOQwByJGxN9w9ulX5kJ7g+QC1CqJeJEAbWFnZf29ix/gKs6Q9tMaI57JrHa6D4j5QHSGCdE= X-Gm-Message-State: AOJu0YwWG1HgCvTnbkghq97DZw2YnCa0IZZDErCJPm/kb7Tp75eZ3mTq zmWu2lk+6W4IOPll343sgH4ethcouhcXH5WDDXzFQk7shPuWeVhU4tfccetLHnfUs404omSvwwB XsHw= X-Google-Smtp-Source: AGHT+IHMBZjh35E+WBgJiOTQ6ndnmv0H588BtbtRXb8/E6ILHBq/YwVg7uR7l0izpdHQd5786uNKiA== X-Received: by 2002:a50:f69a:0:b0:57a:2274:850b with SMTP id 4fb4d7f45d1cf-57a8b708ac0mr3096006a12.24.1717665573104; Thu, 06 Jun 2024 02:19:33 -0700 (PDT) Received: from [10.12.0.137] (81.89.53.154.host.vnet.sk. [81.89.53.154]) by smtp.gmail.com with ESMTPSA id 4fb4d7f45d1cf-57aae0cc1a2sm757587a12.23.2024.06.06.02.19.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 06 Jun 2024 02:19:32 -0700 (PDT) Message-ID: <19574098-0ee0-4194-b11d-9d9e889fca21@pantheon.tech> Date: Thu, 6 Jun 2024 11:19:31 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: =?UTF-8?Q?Juraj_Linke=C5=A1?= Subject: Re: [PATCH v2 1/8] dts: add params manipulation module To: Luca Vizzarro , dev@dpdk.org Cc: Jeremy Spewock , Paul Szczepanek References: <20240326190422.577028-1-luca.vizzarro@arm.com> <20240509112057.1167947-1-luca.vizzarro@arm.com> <20240509112057.1167947-2-luca.vizzarro@arm.com> Content-Language: en-US In-Reply-To: <20240509112057.1167947-2-luca.vizzarro@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org The docstrings are missing the Args: or Returns: sections. On 9. 5. 2024 13:20, Luca Vizzarro 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 main purpose is to make it easier to represent data structures that > map to parameters. Aiding quicker development, while minimising code > bloat. > > Signed-off-by: Luca Vizzarro > Reviewed-by: Paul Szczepanek > --- > dts/framework/params/__init__.py | 274 +++++++++++++++++++++++++++++++ > 1 file changed, 274 insertions(+) > create mode 100644 dts/framework/params/__init__.py > > diff --git a/dts/framework/params/__init__.py b/dts/framework/params/__init__.py > new file mode 100644 > index 0000000000..aa27e34357 > --- /dev/null > +++ b/dts/framework/params/__init__.py > @@ -0,0 +1,274 @@ > +# 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. > +""" This should probably end with command line parameters. > + > +from dataclasses import dataclass, fields > +from enum import Flag > +from typing import Any, Callable, Iterable, Literal, Reversible, TypedDict, cast > + > +from typing_extensions import Self > + > +#: Type for a function taking one argument. > +FnPtr = Callable[[Any], Any] > +#: Type for a switch parameter. > +Switch = Literal[True, None] > +#: Type for a yes/no switch parameter. > +YesNoSwitch = Literal[True, False, None] > + > + > +def _reduce_functions(funcs: Reversible[FnPtr]) -> FnPtr: The static method in the other patch is called compose and does essentially the same thing, right? Can we use the same name (or a similar one)? Also, what is the difference in approaches between the two patches (or, more accurately, the reason behind the difference)? In the other patch, we're returning a dict, here we're returning a function directly. > + """Reduces an iterable of :attr:`FnPtr` from end to start to a composite function. We should make the order of application the same as in the method in other patch, so if we change the order in the first one, we should do the same here. > + > + If the iterable is empty, the created function just returns its fed value back. > + """ > + > + def composite_function(value: Any): The return type is missing. > + for fn in reversed(funcs): > + value = fn(value) > + return value > + > + return composite_function > + > + > +def convert_str(*funcs: FnPtr): The return type is missing. And maybe the name could be better, now it suggests to me that we're converting the __str__ method, but we're actually replacing it, so maybe replace_str() or modify_str()? > + """Decorator that makes the ``__str__`` method a composite function created from its arguments. This should mention that it's a class decorator (and that it replaces or modifies the __str__() method). > + > + The :attr:`FnPtr`s fed to the decorator are executed from right to left > + in the arguments list order. > + > + Example: > + .. code:: python > + > + @convert_str(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__ = _reduce_functions(funcs) > + return original_class > + > + return _class_decorator > + > + > +def comma_separated(values: Iterable[Any]) -> str: > + """Converts 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: > + """Adds round brackets to the input.""" > + return f"({value})" > + > + > +def str_from_flag_value(flag: Flag) -> str: > + """Returns the value from a :class:`enum.Flag` as a string.""" > + return str(flag.value) > + > + > +def hex_from_flag_value(flag: Flag) -> str: > + """Returns the value from a :class:`enum.Flag` converted to hexadecimal.""" > + return hex(flag.value) > + > + > +class ParamsModifier(TypedDict, total=False): > + """Params modifiers dict compatible with the :func:`dataclasses.field` metadata parameter.""" > + > + #: > + Params_value_only: bool > + #: > + Params_short: str > + #: > + Params_long: str > + #: > + Params_multiple: bool > + #: > + Params_convert_value: Reversible[FnPtr] > + > + > +@dataclass > +class Params: > + """Dataclass that renders its fields into command line arguments. > + > + 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 > + this class' metadata modifier functions. > + > + To use fields as switches, set the value to ``True`` to render them. If you > + use a yes/no switch you can also set ``False`` which would render a switch > + prefixed with ``--no-``. Examples: > + > + .. code:: python > + > + interactive: Switch = True # renders --interactive > + numa: YesNoSwitch = False # renders --no-numa > + > + Setting ``None`` will prevent it from being rendered. The :attr:`~Switch` type alias is provided > + for regular switches, whereas :attr:`~YesNoSwitch` 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. > + """ > + > + _suffix = "" > + """Holder of the plain text value of Params when called directly. A suffix for child classes.""" > + > + """========= BEGIN FIELD METADATA MODIFIER FUNCTIONS ========""" > + > + @staticmethod > + def value_only() -> ParamsModifier: As far as I (or my IDE) can tell, this is not used anywhere. What's the purpose of this? > + """Injects the value of the attribute as-is without flag. > + > + Metadata modifier for :func:`dataclasses.field`. > + """ > + return ParamsModifier(Params_value_only=True) > + > + @staticmethod > + def short(name: str) -> ParamsModifier: > + """Overrides any parameter name with the given short option. > + > + Metadata modifier for :func:`dataclasses.field`. > + > + Example: > + .. code:: python > + > + logical_cores: str | None = field(default="1-4", metadata=Params.short("l")) > + > + will render as ``-l=1-4`` instead of ``--logical-cores=1-4``. > + """ > + return ParamsModifier(Params_short=name) > + > + @staticmethod > + def long(name: str) -> ParamsModifier: > + """Overrides the inferred parameter name to the specified one. > + > + Metadata modifier for :func:`dataclasses.field`. > + > + Example: > + .. code:: python > + > + x_name: str | None = field(default="y", metadata=Params.long("x")) > + > + will render as ``--x=y``, but the field is accessed and modified through ``x_name``. > + """ > + return ParamsModifier(Params_long=name) > + > + @staticmethod > + def multiple() -> ParamsModifier: > + """Specifies that this parameter is set multiple times. Must be a list. > + > + Metadata modifier for :func:`dataclasses.field`. > + > + Example: > + .. code:: python > + > + ports: list[int] | None = field( > + default_factory=lambda: [0, 1, 2], > + metadata=Params.multiple() | Params.long("port") > + ) > + > + will render as ``--port=0 --port=1 --port=2``. Note that modifiers can be chained like > + in this example. I'd put the explanation of how modifiers can be chained (and mention they're dicts so the or operator just merges the dicts) into the class docstring. Then we won't have to duplicate the explanation in each method. > + """ > + return ParamsModifier(Params_multiple=True) > + > + @classmethod > + def convert_value(cls, *funcs: FnPtr) -> ParamsModifier: I don't see cls used anywhere, so let's make this static. > + """Takes in a variable number of functions to convert the value text representation. > + > + Metadata modifier for :func:`dataclasses.field`. > + > + The ``metadata`` keyword argument can be used to chain metadata modifiers together. > + > + Functions 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=Params.convert_value(hex) | Params.long("mask") > + ) > + > + will render as ``--mask=0xd``. > + """ > + return ParamsModifier(Params_convert_value=funcs) > + > + """========= END FIELD METADATA MODIFIER FUNCTIONS ========""" > + > + def append_str(self, text: str) -> None: > + """Appends a string at the end of the string representation.""" > + self._suffix += text > + > + def __iadd__(self, text: str) -> Self: > + """Appends a string at the end of the string representation.""" > + self.append_str(text) > + return self > + > + @classmethod > + def from_str(cls, text: str) -> Self: I tried to figure out how self._suffix is used and I ended up finding out this method is not used anywhere. Is that correct? If it's not used, let's remove it. What actually should be the suffix? A an arbitrary string that gets appended to the rendered command line arguments? I guess this would be here so that we can pass an already rendered string? > + """Creates a plain Params object from a string.""" > + obj = cls() > + obj.append_str(text) > + return obj > + > + @staticmethod > + def _make_switch( > + name: str, is_short: bool = False, is_no: bool = False, value: str | None = None > + ) -> str: > + prefix = f"{'-' if is_short else '--'}{'no-' if is_no else ''}" Does is_short work with is_no (that is, if both are True)? Do we need to worry about it if not? > + name = name.replace("_", "-") > + value = f"{' ' if is_short else '='}{value}" if value else "" > + return f"{prefix}{name}{value}" > + > + def __str__(self) -> str: > + """Returns a string of command-line-ready arguments from the class fields.""" > + arguments: list[str] = [] > + > + for field in fields(self): > + value = getattr(self, field.name) > + modifiers = cast(ParamsModifier, field.metadata) > + > + if value is None: > + continue > + > + value_only = modifiers.get("Params_value_only", False) > + if isinstance(value, Params) or value_only: > + arguments.append(str(value)) > + continue > + > + # take the short modifier, or the long modifier, or infer from field name > + switch_name = modifiers.get("Params_short", modifiers.get("Params_long", field.name)) > + is_short = "Params_short" in modifiers > + > + if isinstance(value, bool): > + arguments.append(self._make_switch(switch_name, is_short, is_no=(not value))) > + continue > + > + convert = _reduce_functions(modifiers.get("Params_convert_value", [])) > + multiple = modifiers.get("Params_multiple", False) > + > + values = value if multiple else [value] > + for value in values: > + arguments.append(self._make_switch(switch_name, is_short, value=convert(value))) > + > + if self._suffix: > + arguments.append(self._suffix) > + > + return " ".join(arguments)