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

[John Snow <jsnow@redhat.com>]

On 4/27/21 3:15 AM, Markus Armbruster wrote:
> 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?
> 

If it works for you, it works for me!

--js