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 2DE334415D; Wed, 5 Jun 2024 14:19:18 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 150C940DCA; Wed, 5 Jun 2024 14:19:18 +0200 (CEST) Received: from mail-lf1-f46.google.com (mail-lf1-f46.google.com [209.85.167.46]) by mails.dpdk.org (Postfix) with ESMTP id 7D0F24067B for ; Wed, 5 Jun 2024 14:19:16 +0200 (CEST) Received: by mail-lf1-f46.google.com with SMTP id 2adb3069b0e04-52b93370ad0so6053879e87.2 for ; Wed, 05 Jun 2024 05:19:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pantheon.tech; s=google; t=1717589956; x=1718194756; 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=9vtw0/uWuTx9IE9qQeBc0MDrvbkfhQRQ+kkGHF8aHGE=; b=mTc9qYnminmgWpo4aR8+WpU2fO27zxAC5sABl0k3ksq+bJM9j7KcWLV6xCeblgzf6H 25lSinnCbs86V7mfGDiydwEb96dnPAFOPEM5IRVLOVOktdPzoijmXitPXRbmA8C4TrjA zntOeYnd0vjMoyDEOJctBpWWYgyBsNSdOWiBh8BThQJNQOpBmFayJNbBJE6se7ivBMci kMgg1q/wopuCyDB1Z9LeRNlJTLs0L0vq7Hy61lui5ljrCizAX3jitxkdu1eYSSqExTIM oRrqwm41eMnQXmRa0/T8TaqnngbnVISKxAGshknwkJ5wpuLV2L23xz3mNfmm1fkhtpd8 6EYg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1717589956; x=1718194756; 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=9vtw0/uWuTx9IE9qQeBc0MDrvbkfhQRQ+kkGHF8aHGE=; b=EpUeK2AihU63/Wc0ARM0s6MHIEL61JvGqRyqd0P+PwbNqfamuHjdiIDdyfLV3JuOHg 8/SjfDr6xNkjJ3RcWWBUU2DiuydREM7osNXv+wyRf9zV9Pyl+j4iJH6+vo6ZuNAnee/t +mKeYF5FMQ9Ciem4VFqGfsB/nasAbM9lOFUl4tSM/uzjBW8U8zkpcQGS++6fpYYlW1Ws r3vyJZm4WqXIMTqg/G7z4Tl8WI82Xei8QZocH/xw60n8TcAL4t0/KE0q9+k4D+3H7Emb ezQjcTwZsoD8mzAkM1X4AkaKD7Nb7FCUTeIaZeOV4iF49z2Q/SbzQ+OY2ipeCh6RVHgX A9eA== X-Forwarded-Encrypted: i=1; AJvYcCWMAXG3VHfqsbKDHg2Iu+MsWKQsKzrcYfGuJ/8Q+165um3E4CowPT8oRZOiYkuEUuJTJP68HyArQicnZms= X-Gm-Message-State: AOJu0YxuLUiPVSblXnl5AR1pLPwPQYo2a+ua0CCXvlwi87YdGZSeuqRi aXz+pdimFkC84rih7JQGBU5Wkbk3GQ1Yz1uNBWiBlBh7wlnWOki7pOB8ErYaN3s= X-Google-Smtp-Source: AGHT+IF2Wo5vfnmlF95/GobH3Q9fK6B0SL0EGBNjwPPHJJb0dC7mH5qdjEmrEa7FgMqLQbT4y72FDw== X-Received: by 2002:a19:4358:0:b0:52b:8a71:2688 with SMTP id 2adb3069b0e04-52bab50b6c0mr1862737e87.61.1717589955327; Wed, 05 Jun 2024 05:19:15 -0700 (PDT) Received: from [192.168.1.113] ([84.245.121.236]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-a68bdf174bbsm612697466b.222.2024.06.05.05.19.14 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 05 Jun 2024 05:19:14 -0700 (PDT) Message-ID: Date: Wed, 5 Jun 2024 14:19:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 3/5] dts: add parsing utility module To: Luca Vizzarro , dev@dpdk.org Cc: Jeremy Spewock , Paul Szczepanek References: <20240412111136.3470304-1-luca.vizzarro@arm.com> <20240509112635.1170557-1-luca.vizzarro@arm.com> <20240509112635.1170557-4-luca.vizzarro@arm.com> <685b9f60-7b5d-407f-be26-69115e23a9d2@pantheon.tech> Content-Language: en-US From: =?UTF-8?Q?Juraj_Linke=C5=A1?= In-Reply-To: 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 >>> + >>> +    """============ BEGIN PARSER FUNCTIONS ============""" >>> + >>> +    @staticmethod >>> +    def compose(f: Callable, parser_fn: ParserFn) -> ParserFn: >>> +        """Makes a composite parser function. >>> + >>> +        The parser function is run and if a non-None value was >>> returned, f is called with it. >> >> Let's use `parser_fn` instead of "The parser function". Let's also put >> f into backticks. It's also peculiar that we're passing the functions >> in the reverse order - first the second one is applied and then the >> first one is applied. >> > > Ack. > > compose(f, g) is equivalent of f(g(..)). It follows a syntactical order, > rather than a calling one. I don't have a preference on ordering either > way, so anything is fine by me. > I'd prefer to have the calling order. You could also include that the resulting function is f(g()). >>> +        Otherwise the function returns early with None. >>> + >>> +        Metadata modifier for :func:`dataclasses.field`. >> >> This sentence is all alone here and I don't understand what it's >> saying without more context. > > Similarly to what was previously done with the Params class, this is > just to signify that this function is modifies the metadata argument of > dataclasses.field. It's just a note. I guess this could actually be in > the Returns section, and it'd be clearer that way. > Ok, let's see how that reads. >>> +    @staticmethod >>> +    def find( >>> +        pattern: str | re.Pattern[str], >>> +        flags: re.RegexFlag = re.RegexFlag(0), >>> +        named: bool = False, >>> +    ) -> ParserFn: >>> +        """Makes a parser function that finds a regular expression >>> match in the text. >>> + >>> +        If the pattern has capturing groups, it returns None if no >>> match was found. If the pattern >>> +        has only one capturing group and a match was found, its >>> value is returned. If the pattern >>> +        has no capturing groups then either True or False is >>> returned if the pattern had a match or >>> +        not. >>> + >> >> This description would be a better fit in a Returns: section. The body >> could explain the various scenarios in which the function can be used. >> At least I imagine there are various scenarios based on the different >> things the function returns, but I don't really want to go around >> digging in the code to verify that. :-) > > Returns is about what the `find` returns, but it's not what it actually > returns. `find` returns a dictionary compatible with > `dataclasses.field`'s metadata argument, which contains the actual _find > function. Hence why I wrote that segment in the body. I think it would > be confusing to put this description in the returns. I agree that this > should be clarified though. > They way it's phrased it looks like you're describing what find() returns, but I get that it returns a function that returns what you described. It could be easily clarified in the returns section though: Returns: A function that returns . >>> +    @classmethod >> >> Is there a reason why find_int() is a classmethod while the rest are >> staticmethods? Looks like it could also be a staticmethod. > > Class methods are pretty much static methods that receive a reference to > the class. In this case it's a class method as we are calling another > class method: cls.find(..). The @staticmethod equivalent would be > explicitly specifying the class and the method we want to use. > I was wondering about this because it doesn't need to be a classmethod, as we're calling a static method from within and as you mention, we can just specify the class with the method (i.e. TextParser.find()), so my thoughts were why is find() deviating from the rest of the methods when it doesn't need to. This doesn't really matter though, it's going to be called the same from outside the class, so I'll leave this up to you. :-) >>> +    def find_int( >>> +        cls, >>> +        pattern: str | re.Pattern[str], >>> +        flags: re.RegexFlag = re.RegexFlag(0), >>> +        int_base: int = 0, >>> +    ) -> ParserFn: >>> +        """Makes a parser function that converts the match of >>> :meth:`~find` to int. >>> + >>> +        This function is compatible only with a pattern containing >>> one capturing group. >>> + >>> +        Metadata modifier for :func:`dataclasses.field`. >>> + >>> +        Args: >>> +            pattern: the regular expression pattern >>> +            flags: the regular expression flags >>> +            int_base: the base of the number to convert from >>> +        Raises: >>> +            RuntimeError: if the pattern does not have exactly one >>> capturing group >>> +        """ >>> +        if isinstance(pattern, str): >>> +            pattern = re.compile(pattern, flags) >>> + >>> +        if pattern.groups != 1: >>> +            raise RuntimeError("only one capturing group is allowed >>> with this parser function") >>> + >> >> Have you considered using a subclass of DTSError so that we can add a >> severity to this error? The severity is there to "rank" DTS errors >> from worst to least worst. > > No, I actually didn't think of it. This error should only occur out of > development error, and it should never be triggered in practice. Do you > think it should be used? > I err on the side of using DTSErrors, as it gives automated tools a possibly useful way of telling the severity of the error (from the return code). A development error like this could be easily identifiable in CI then and we could maybe produce a helpful message based on that. But maybe we can talk about this Patrick and his team (how usable the return code is and what the severities should be). >>> +        return cls.compose(partial(int, base=int_base), >>> cls.find(pattern)) >>> + >>> +    """============ END PARSER FUNCTIONS ============""" >>> + >>> +    @classmethod >>> +    def parse(cls, text: str) -> Self: >> >> Would converting this into __init__(self, text: str) work? Sounds like >> we could just use "for field in fields(self)" and then setattr() to >> populate the variables. > > I am not in favour of overriding the constructor, as I see the parsing > ability as an extension to the class. Well, the class should primarily do the parsing (based on its name), so everything else is an extension in my mind. > Nonetheless, this would make sense > if we think that this would be used exclusively for objects that need to > be parsed in order to be constructed. I purposefully left the > flexibility open, but if we don't think it'll ever be needed, then it's > not a game changer to me. > Everything about the class indicates (or outright says) that parsing should be the only purpose. I can't really imagine what else we could add (in this state, an instance before calling parse() would be just the text with fields containing function in the metadata needed for parsing - what else is there to do with this data?). Can you elaborate if you can think of something? And again, that said, it doesn't matter much, but I like the constructor as it really doesn't look like the class could (or should) do anything else than parsing text.