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 EA8B04548B; Tue, 18 Jun 2024 10:55:08 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8FDEB40DC9; Tue, 18 Jun 2024 10:55:08 +0200 (CEST) Received: from mail-wm1-f49.google.com (mail-wm1-f49.google.com [209.85.128.49]) by mails.dpdk.org (Postfix) with ESMTP id 9FFF440684 for ; Tue, 18 Jun 2024 10:55:06 +0200 (CEST) Received: by mail-wm1-f49.google.com with SMTP id 5b1f17b1804b1-4217d451f69so42741445e9.0 for ; Tue, 18 Jun 2024 01:55:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1718700906; x=1719305706; darn=dpdk.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=mykclcsvAVVivWyyP+JunptRR1u4QZxRfGdspzZkk7c=; b=krqQvIHafkXzGX0N/5Nn0ztVK8b05GIMFi8ci1Yi9ICe/YvdG2fCgUZpN4BytgM714 57a58ko6xAvbWtxhH56DAsAs9R5VEY3M4Y/kJNzj+FVRDUs0J74VPgruhO6C46DcYOsI 2Tx0IIHhio6ZKL564jN9IifKIxLuGUvVBHlgX3McfGfja1lPvyGtozUWFa/o18PGH+Ur SfvG7Vor6yxRMeN+4hbJKUSdQoMYFUupQV8dVmE2vXBdkCJEbqoN7b+8WQHd01O9BBQ9 3GxvOIBBwcxYhoKs9dooe1UwSdBNEs4anepjlEJqm9brDtCwDJM0eutsEQbrpyev0cdq jQRw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718700906; x=1719305706; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=mykclcsvAVVivWyyP+JunptRR1u4QZxRfGdspzZkk7c=; b=XXd/g9szV2mYw0JIfu3umxDxEJ10/bJZ2EbBk7VOFDcujuaFwgXT6hPUyw1BNp78gQ 4WEfoNqZDiBH0GYO/3/ZFjsbttkJZbM/wwBAb7JeB1tbOONZtGXku86at0pt+bsZ/j0V Pv+ur6Skm1OTQhYcksHdl+SUyyLL4s8fUse8rCngI+2g2BWoig7kG6lKHdOhRBsjHXKy XdchjYaNRXke3Ohgi1+LDX+qvuyJ5vFo7FYaL4hGJtfnkmIAZDNjeJVayr9syBcGlJ+u es3/pd60XXedqrmvqtoz6yF2jXmNRt9Y5nhZmsYMK6MVt0bNdZRIB3+i1vQ/e5tRuCnT WvlA== X-Forwarded-Encrypted: i=1; AJvYcCVvQZcOn9EyVkN5wKqqiGdoB8dSp9kxKX5bR99v3u/N13b5BN7HzolWQaUQdspPixmXNIqt4XLIPl5vK7A= X-Gm-Message-State: AOJu0Yxn+VUr76gA6JIHbnEYmsgItsZn9pwLUvq0s4jqdFca0nmGn2hQ voXzMTzAGEs+t5wWuQ4QMds4gFFTU7VRK50Xaa2YSmkcevNIsFtrqZMwG4ak+zbLBudCji/VUHs QEgo= X-Google-Smtp-Source: AGHT+IEDWwkYX765SQ1v3uh6dXCYTQHak3D7n1pqTE4c6XtelWZZYHpAybsKG1uY30E0tBTwTUjmwA== X-Received: by 2002:a05:600c:46ce:b0:421:7aa1:441 with SMTP id 5b1f17b1804b1-42304822753mr98164085e9.6.1718700906050; Tue, 18 Jun 2024 01:55:06 -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 5b1f17b1804b1-42286fe9184sm215235535e9.13.2024.06.18.01.55.05 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 18 Jun 2024 01:55:05 -0700 (PDT) Message-ID: Date: Tue, 18 Jun 2024 10:55:04 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird 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> <19574098-0ee0-4194-b11d-9d9e889fca21@pantheon.tech> <7182b9ba-1671-4707-9b2b-dfdb0fcb9995@arm.com> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: <7182b9ba-1671-4707-9b2b-dfdb0fcb9995@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 On 17. 6. 2024 13:44, Luca Vizzarro wrote: > On 06/06/2024 10:19, Juraj Linkeš wrote: >> 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. > > They are essentially different. This one is as it's called quite plainly > a function reduction. Can be used in any context in reality. > > The other one is tighter and has some specific controls (exit early if > None), and is directly represented as a dictionary as that's the only > intended way of consumption. > This is a generic explanation. I had to go back to the code to find the reason for the difference. The dict is tailored to be used with the dataclass field's metadata argument and the reduce function is used with functions gotten from the metadata. At this point, the question is, why the difference when it seems we're trying to do the same thing (apply multiple functions through metadata), but slightly differently? >>> +    """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. > > While I don't think it feels any natural coding-wise (yet again, a > matter of common readability to the developer vs how it's actually run), > I won't object as I don't have a preference. > >>> + >>> +    If the iterable is empty, the created function just returns its >>> fed value back. >>> +    """ >>> + >>> +    def composite_function(value: Any): >> >> The return type is missing. > > I will remove types from the decorator functions as it adds too much > complexity and little advantage. I wasn't able to easily resolve with > mypy. Especially in conjunction with modify_str, where mypy complains a > lot about __str__ being treated as an unbound method/plain function that > doesn't take self. > We can make this a policy - don't add return types to decorators. But I'm curious, what does mypy say when you use just Callable? >>> +    _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? > > I guess this is no longer in use at the moment. It could still be used > for positional arguments in the future. But will remove for now. > >>> +    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. > > It is used through Params.from_str. It is used transitionally in the > next commits. > Can we remove it with the last usage of it being removed? As far as I understand, there's no need for the method with all commits applied. >> 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? > > As we already discussed and agreed before, this is just to use Params > plainly with arbitrary text. It is treated as a suffix, because if > Params is inherited this stays, and then it can either be a prefix or a > suffix in that case. > >>> +        """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? > > They should not work together, and no it is not enforced. But finally > that's up to the command line interface we are modelling the parameters > against. It is not a problem in reality. >