Re: [PATCH 11/22] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 4/25/21 8:32 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> TypeGuards wont exist in Python proper until 3.10. Ah well. We can hack
>>> up our own by declaring this function to return the type we claim it
>>> checks for and using this to safely downcast object -> List[str].
>>>
>>> In so doing, I bring this function in-line under _pragma so it can use
>>> the 'info' object in its closure. Having done this, _pragma also now
>>> no longer needs to take a 'self' parameter, so drop it.
>>>
>>> Rename it to just _check(), to help us out with the line-length -- and
>>> now that it's contained within _pragma, it is contextually easier to see
>>> how it's used anyway -- especially with types.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> ---
>>>
>>> I left (name, value) as args to avoid creating a fully magic "macro",
>>> though, I thought this was too weird:
>>>
>>>      info.pragma.foobar = _check()
>>>
>>> and it looked more reasonable as:
>>>
>>>      info.pragma.foobar = _check(name, value)
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/parser.py | 26 +++++++++++++-------------
>>>   1 file changed, 13 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>>> index 16fd36f8391..d02a134aae9 100644
>>> --- a/scripts/qapi/parser.py
>>> +++ b/scripts/qapi/parser.py
>>> @@ -17,6 +17,7 @@
>>>   from collections import OrderedDict
>>>   import os
>>>   import re
>>> +from typing import List
>>>   
>>>   from .common import match_nofail
>>>   from .error import QAPISemError, QAPISourceError
>>> @@ -151,28 +152,27 @@ def _include(include, info, incl_fname, 
>>> previously_included):
>>>               ) from err
>>>   
>>>       @staticmethod
>>> -    def _check_pragma_list_of_str(name, value, info):
>>> -        if (not isinstance(value, list)
>>> -                or any([not isinstance(elt, str) for elt in value])):
>>> -            raise QAPISemError(
>>> -                info,
>>> -                "pragma %s must be a list of strings" % name)
>>> +    def _pragma(name, value, info):
>>> +
>>> +        def _check(name, value) -> List[str]:
>>> +            if (not isinstance(value, list) or
>>> +                    any([not isinstance(elt, str) for elt in value])):
>>> +                raise QAPISemError(
>>> +                    info,
>>> +                    "pragma %s must be a list of strings" % name)
>>> +            return value
>>>   
>>> -    def _pragma(self, name, value, info):
>>>           if name == 'doc-required':
>>>               if not isinstance(value, bool):
>>>                   raise QAPISemError(info,
>>>                                      "pragma 'doc-required' must be 
>>> boolean")
>>>               info.pragma.doc_required = value
>>>           elif name == 'command-name-exceptions':
>>> -            self._check_pragma_list_of_str(name, value, info)
>>> -            info.pragma.command_name_exceptions = value
>>> +            info.pragma.command_name_exceptions = _check(name, value)
>>>           elif name == 'command-returns-exceptions':
>>> -            self._check_pragma_list_of_str(name, value, info)
>>> -            info.pragma.command_returns_exceptions = value
>>> +            info.pragma.command_returns_exceptions = _check(name, value)
>>>           elif name == 'member-name-exceptions':
>>> -            self._check_pragma_list_of_str(name, value, info)
>>> -            info.pragma.member_name_exceptions = value
>>> +            info.pragma.member_name_exceptions = _check(name, value)
>>>           else:
>>>               raise QAPISemError(info, "unknown pragma '%s'" % name)
>> 
>> While I appreciate the terseness, I'm not sure I like the generic name
>> _check() for checking one of two special cases, namely "list of string".
>> The other case being "boolean".  We could acquire more cases later.
>> 
>
> Yeah, sorry, just trying to make the line fit ...

I understand!

> The important thing is that we need to make sure this routine returns 
> some known type. It's just that the block down here has very long lines.
>
> Recommendations?

Moving the helper into _pragma() lets us drop shorten its name.  Still
too long to fit the line:

            info.pragma.command_returns_exceptions = check_list_str(name, value)

We could break the line in the argument list:

            info.pragma.command_returns_exceptions = check_list_str(name,
                                                                    value)

or

            info.pragma.command_returns_exceptions = check_list_str(
                                                            name, value)

Not exactly pretty.

We could shorten the assignment's target:

            pragma.command_returns_exceptions = check_list_str(name, value)

with

        pragma.info = pragma

before the conditional.  I'm not too fond of creating aliases, but this
one looks decent to me.  What do you think?