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

[Markus Armbruster]

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.