John Snow <js...@redhat.com> writes:
> On 4/25/21 8:32 AM, Markus Armbruster wrote:
>> John Snow <js...@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 <js...@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 <js...@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?
