| Message ID | 20210422030720.3685766-12-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: static typing conversion, pt5a | expand |
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.
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 ... 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?
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?
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
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)