DPDK patches and discussions
 help / color / mirror / Atom feed
From: Luca Vizzarro <Luca.Vizzarro@arm.com>
To: "Juraj Linkeš" <juraj.linkes@pantheon.tech>, dev@dpdk.org
Cc: Jeremy Spewock <jspewock@iol.unh.edu>,
	Paul Szczepanek <paul.szczepanek@arm.com>
Subject: Re: [PATCH v2 3/5] dts: add parsing utility module
Date: Wed, 5 Jun 2024 11:35:42 +0100	[thread overview]
Message-ID: <ec7e79d0-fb61-42e4-8bfc-53dc9b45bc61@arm.com> (raw)
In-Reply-To: <685b9f60-7b5d-407f-be26-69115e23a9d2@pantheon.tech>

On 04/06/2024 16:13, Juraj Linkeš wrote:
> I'd like to see a high level explanation of what the key pieces of the 
> parsing are and how they're tied to the implementation:
> 1. The text we're about to parse, passed to the instance of a subclass
> 2. What we're parsing, the fields of the subclass
> 3. How we're parsing the fields, the functions of TextParser
> 
> This could be part of the example or mentioned before the example or in 
> the class. I had to study the code to understand the API, so that should 
> be better documented.
Ack.

>> +@dataclass
>> +class TextParser(ABC):
>> +    """Helper abstract dataclass that parses a text according to the 
>> fields' rules.
>> +
>> +    This class provides a selection of parser functions and a 
>> function to compose generic functions
>> +    with parser functions. Parser functions are designed to be passed 
>> to the fields' metadata param
>> +    to enable parsing.
>> +    """
> 
> We should add that this class should be subclassed; basically what I've 
> laid out above so that devs know how to actually use the class. I'm not 
> sure which is the better place to put it, but probably here.
Ack.
>> +
>> +    """============ 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.

>> +        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.

>> +
>> +        Args:
>> +            f: the function to apply to the parser's result
> 
> This now refers to just parser's results instead of parser functions's 
> result, which is confusing. But let's also use `parser_fn` here.
> 
> Also, the arg descriptions should end with a dot. And start with a 
> capital letter.
> 
> https://www.sphinx-doc.org/en/master/usage/extensions/example_google.html#example-google
> 

Ack.

>> +            parser_fn: the dictionary storing the parser function
>> +        """
> 
> The docstring is missing a Returns: section.

Ack.

>> +        g = parser_fn["TextParser_fn"]
> 
> This is a matter of preference, but I like more descriptive variable 
> names, something like initial_func. And then I'd change f to 
> subsequent_func, which would make the function signature a bit more 
> readable.

Ack.

>> +    @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.

> Also a note: what does the function return if the pattern has capturing 
> groups and a match is found?

Ha! The implicit answer is the captured groups, but yes it should be 
added. Nice catch.

>> +        Metadata modifier for :func:`dataclasses.field`.
>> +
>> +        Args:
>> +            pattern: the regular expression pattern
>> +            flags: the regular expression flags. Not used if the 
>> given pattern is already compiled
>> +            named: if set to True only the named capture groups will 
>> be returned as a dictionary
>> +        """
> 
> This is also missing the Returns: sections.

Ack.

>> +        if isinstance(pattern, str):
>> +            pattern = re.compile(pattern, flags)
>> +
>> +        def _find(text: str) -> Any:
>> +            m = pattern.search(text)
>> +            if m is None:
>> +                return None if pattern.groups > 0 else False
>> +
>> +            if pattern.groups == 0:
>> +                return True
>> +
>> +            if named:
>> +                return m.groupdict()
>> +
>> +            matches = m.groups()
>> +            if len(matches) == 1:
>> +                return matches[0]
>> +
>> +            return matches
>> +
>> +        return ParserFn(TextParser_fn=_find)
>> +
>> +    @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.

>> +    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?

>> +        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. 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.

>> +        for field in fields(cls):
>> +            parse = cast(ParserFn, field.metadata).get("TextParser_fn")
>> +            if parse is None:
>> +                continue
>> +
>> +            value = parse(text)
>> +            if value is not None:
>> +                fields_values[field.name] = value
> 
> If we convert the method into a constructor, we would just use setattr() 
> here.

Ack.

  reply	other threads:[~2024-06-05 10:35 UTC|newest]

Thread overview: 95+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-12 11:11 [PATCH 0/5] dts: testpmd show port info/stats Luca Vizzarro
2024-04-12 11:11 ` [PATCH 1/5] dts: fix InteractiveShell command prompt filtering Luca Vizzarro
2024-04-16  8:40   ` Juraj Linkeš
2024-04-16 12:12     ` Luca Vizzarro
2024-04-17 13:06       ` Juraj Linkeš
2024-04-17 14:17         ` Luca Vizzarro
2024-04-18  6:31           ` Juraj Linkeš
2024-04-29 16:16             ` Jeremy Spewock
2024-04-12 11:11 ` [PATCH 2/5] dts: skip first line of send_command output Luca Vizzarro
2024-04-16  8:48   ` Juraj Linkeš
2024-04-16 12:15     ` Luca Vizzarro
2024-04-17 13:18       ` Juraj Linkeš
2024-04-29 15:18         ` Jeremy Spewock
2024-06-14 18:58           ` Nicholas Pratte
2024-04-12 11:11 ` [PATCH 3/5] dts: add parsing utility module Luca Vizzarro
2024-04-16  8:59   ` Juraj Linkeš
2024-04-16 12:16     ` Luca Vizzarro
2024-04-29 16:15   ` Jeremy Spewock
2024-04-30 10:49     ` Luca Vizzarro
2024-04-30 20:03       ` Jeremy Spewock
2024-04-12 11:11 ` [PATCH 4/5] dts: add `show port info` command to TestPmdShell Luca Vizzarro
2024-04-16  9:03   ` Juraj Linkeš
2024-04-16 12:24     ` Luca Vizzarro
2024-04-17 13:22       ` Juraj Linkeš
2024-04-17 14:25         ` Luca Vizzarro
2024-04-17 15:29           ` Luca Vizzarro
2024-04-18  6:41             ` Juraj Linkeš
2024-04-18 10:52               ` Luca Vizzarro
2024-04-12 11:11 ` [PATCH 5/5] dts: add `show port stats` " Luca Vizzarro
2024-04-16  9:04   ` Juraj Linkeš
2024-04-29 15:54   ` Jeremy Spewock
2024-04-30 10:51     ` Luca Vizzarro
2024-05-09 11:26 ` [PATCH v2 0/5] dts: testpmd show port info/stats Luca Vizzarro
2024-05-09 11:26   ` [PATCH v2 1/5] dts: fix InteractiveShell command prompt filtering Luca Vizzarro
2024-05-31 21:06     ` Jeremy Spewock
2024-06-04 13:57     ` Juraj Linkeš
2024-05-09 11:26   ` [PATCH v2 2/5] dts: skip first line of send command output Luca Vizzarro
2024-05-31 21:06     ` Jeremy Spewock
2024-06-04 13:58     ` Juraj Linkeš
2024-05-09 11:26   ` [PATCH v2 3/5] dts: add parsing utility module Luca Vizzarro
2024-05-31 21:06     ` Jeremy Spewock
2024-06-04 15:13     ` Juraj Linkeš
2024-06-05 10:35       ` Luca Vizzarro [this message]
2024-06-05 12:19         ` Juraj Linkeš
2024-06-05 13:00           ` Luca Vizzarro
2024-05-09 11:26   ` [PATCH v2 4/5] dts: add `show port info` command to TestPmdShell Luca Vizzarro
2024-05-31 21:06     ` Jeremy Spewock
2024-06-03  9:11       ` Luca Vizzarro
2024-06-04 15:40     ` Juraj Linkeš
2024-06-05 11:03       ` Luca Vizzarro
2024-05-09 11:26   ` [PATCH v2 5/5] dts: add `show port stats` " Luca Vizzarro
2024-05-20 14:26     ` Nicholas Pratte
2024-05-21 15:00       ` Luca Vizzarro
2024-05-31 21:07     ` Jeremy Spewock
2024-06-04 15:44     ` Juraj Linkeš
2024-06-05 11:04       ` Luca Vizzarro
2024-06-05 14:44 ` [PATCH v3 0/5] dts: testpmd show port info/stats Luca Vizzarro
2024-06-05 14:45   ` [PATCH v3 1/5] dts: fix InteractiveShell command prompt filtering Luca Vizzarro
2024-06-05 14:45   ` [PATCH v3 2/5] dts: skip first line of send command output Luca Vizzarro
2024-06-05 14:45   ` [PATCH v3 3/5] dts: add parsing utility module Luca Vizzarro
2024-06-05 14:45   ` [PATCH v3 4/5] dts: add `show port info` command to TestPmdShell Luca Vizzarro
2024-06-05 14:45   ` [PATCH v3 5/5] dts: add `show port stats` " Luca Vizzarro
2024-06-06  9:17 ` [PATCH v4 0/5] dts: testpmd show port info/stats Luca Vizzarro
2024-06-06  9:17   ` [PATCH v4 1/5] dts: fix InteractiveShell command prompt filtering Luca Vizzarro
2024-06-06 18:52     ` Jeremy Spewock
2024-06-06  9:17   ` [PATCH v4 2/5] dts: skip first line of send command output Luca Vizzarro
2024-06-06 18:52     ` Jeremy Spewock
2024-06-06  9:17   ` [PATCH v4 3/5] dts: add parsing utility module Luca Vizzarro
2024-06-06 18:52     ` Jeremy Spewock
2024-06-06 21:14       ` Luca Vizzarro
2024-06-06  9:17   ` [PATCH v4 4/5] dts: add `show port info` command to TestPmdShell Luca Vizzarro
2024-06-06 18:52     ` Jeremy Spewock
2024-06-06  9:17   ` [PATCH v4 5/5] dts: add `show port stats` " Luca Vizzarro
2024-06-06 18:53     ` Jeremy Spewock
2024-06-06 21:34 ` [PATCH v5 0/5] dts: testpmd show port info/stats Luca Vizzarro
2024-06-06 21:34   ` [PATCH v5 1/5] dts: fix InteractiveShell command prompt filtering Luca Vizzarro
2024-06-07 11:15     ` Juraj Linkeš
2024-06-07 13:10     ` Jeremy Spewock
2024-06-14 20:05     ` Nicholas Pratte
2024-06-06 21:34   ` [PATCH v5 2/5] dts: skip first line of send command output Luca Vizzarro
2024-06-07 11:16     ` Juraj Linkeš
2024-06-07 13:10     ` Jeremy Spewock
2024-06-06 21:34   ` [PATCH v5 3/5] dts: add parsing utility module Luca Vizzarro
2024-06-07 11:16     ` Juraj Linkeš
2024-06-07 13:11     ` Jeremy Spewock
2024-06-14 17:39     ` Nicholas Pratte
2024-06-06 21:34   ` [PATCH v5 4/5] dts: add `show port info` command to TestPmdShell Luca Vizzarro
2024-06-07 11:16     ` Juraj Linkeš
2024-06-07 13:11     ` Jeremy Spewock
2024-06-14 17:36     ` Nicholas Pratte
2024-06-06 21:34   ` [PATCH v5 5/5] dts: add `show port stats` " Luca Vizzarro
2024-06-07 11:16     ` Juraj Linkeš
2024-06-07 13:11     ` Jeremy Spewock
2024-06-14 17:34     ` Nicholas Pratte
2024-06-20  3:04   ` [PATCH v5 0/5] dts: testpmd show port info/stats Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ec7e79d0-fb61-42e4-8bfc-53dc9b45bc61@arm.com \
    --to=luca.vizzarro@arm.com \
    --cc=dev@dpdk.org \
    --cc=jspewock@iol.unh.edu \
    --cc=juraj.linkes@pantheon.tech \
    --cc=paul.szczepanek@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).