| Message ID | 20210325060356.4040114-14-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: static typing conversion, pt3 | expand |
John Snow <jsnow@redhat.com> writes: > This is a small rewrite to address some minor style nits. > > Don't compare against the empty list to check for the empty condition, and > move the normalization forward to unify the check on the now-normalized > structure. > > With the check unified, the local nested function isn't needed anymore > and can be brought down into the normal flow of the function. With the > nesting level changed, shuffle the error strings around a bit to get > them to fit in 79 columns. > > Note: although ifcond is typed as Sequence[str] elsewhere, we *know* that > the parser will produce real, bona-fide lists. It's okay to check > isinstance(ifcond, list) here. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/expr.py | 32 ++++++++++++++------------------ > 1 file changed, 14 insertions(+), 18 deletions(-) > > diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py > index ea9d39fcf2..5921fa34ab 100644 > --- a/scripts/qapi/expr.py > +++ b/scripts/qapi/expr.py > @@ -144,30 +144,26 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: > > def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: > > - def check_if_str(ifcond: object) -> None: > - if not isinstance(ifcond, str): > - raise QAPISemError( > - info, > - "'if' condition of %s must be a string or a list of strings" > - % source) > - if ifcond.strip() == '': > - raise QAPISemError( > - info, > - "'if' condition '%s' of %s makes no sense" > - % (ifcond, source)) > - > ifcond = expr.get('if') > if ifcond is None: > return > + > if isinstance(ifcond, list): > - if ifcond == []: > + if not ifcond: > raise QAPISemError( > - info, "'if' condition [] of %s is useless" % source) > - for elt in ifcond: > - check_if_str(elt) > + info, f"'if' condition [] of {source} is useless") Unrelated change from interpolation to formatted string literal. > else: > - check_if_str(ifcond) > - expr['if'] = [ifcond] > + # Normalize to a list > + ifcond = expr['if'] = [ifcond] > + > + for elt in ifcond: > + if not isinstance(elt, str): > + raise QAPISemError(info, ( > + f"'if' condition of {source}" > + " must be a string or a list of strings")) > + if not elt.strip(): > + raise QAPISemError( > + info, f"'if' condition '{elt}' of {source} makes no sense") Likewise. I like formatted string literals, they're often easier to read than interpolation. But let's try to keep patches focused on their stated purpose. I'd gladly consider a series that convers to formatted strings wholesale. But I guess we better finish the typing job, first. > > > def normalize_members(members: object) -> None:
On 3/25/21 11:15 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> This is a small rewrite to address some minor style nits. >> >> Don't compare against the empty list to check for the empty condition, and >> move the normalization forward to unify the check on the now-normalized >> structure. >> >> With the check unified, the local nested function isn't needed anymore >> and can be brought down into the normal flow of the function. With the >> nesting level changed, shuffle the error strings around a bit to get >> them to fit in 79 columns. >> >> Note: although ifcond is typed as Sequence[str] elsewhere, we *know* that >> the parser will produce real, bona-fide lists. It's okay to check >> isinstance(ifcond, list) here. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/expr.py | 32 ++++++++++++++------------------ >> 1 file changed, 14 insertions(+), 18 deletions(-) >> >> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py >> index ea9d39fcf2..5921fa34ab 100644 >> --- a/scripts/qapi/expr.py >> +++ b/scripts/qapi/expr.py >> @@ -144,30 +144,26 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: >> >> def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: >> >> - def check_if_str(ifcond: object) -> None: >> - if not isinstance(ifcond, str): >> - raise QAPISemError( >> - info, >> - "'if' condition of %s must be a string or a list of strings" >> - % source) >> - if ifcond.strip() == '': >> - raise QAPISemError( >> - info, >> - "'if' condition '%s' of %s makes no sense" >> - % (ifcond, source)) >> - >> ifcond = expr.get('if') >> if ifcond is None: >> return >> + >> if isinstance(ifcond, list): >> - if ifcond == []: >> + if not ifcond: >> raise QAPISemError( >> - info, "'if' condition [] of %s is useless" % source) >> - for elt in ifcond: >> - check_if_str(elt) >> + info, f"'if' condition [] of {source} is useless") > > Unrelated change from interpolation to formatted string literal. > >> else: >> - check_if_str(ifcond) >> - expr['if'] = [ifcond] >> + # Normalize to a list >> + ifcond = expr['if'] = [ifcond] >> + >> + for elt in ifcond: >> + if not isinstance(elt, str): >> + raise QAPISemError(info, ( >> + f"'if' condition of {source}" >> + " must be a string or a list of strings")) >> + if not elt.strip(): >> + raise QAPISemError( >> + info, f"'if' condition '{elt}' of {source} makes no sense") > > Likewise. > > I like formatted string literals, they're often easier to read than > interpolation. But let's try to keep patches focused on their stated > purpose. > > I'd gladly consider a series that convers to formatted strings > wholesale. But I guess we better finish the typing job, first. > I am dreaming of a lush meadow.
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py index ea9d39fcf2..5921fa34ab 100644 --- a/scripts/qapi/expr.py +++ b/scripts/qapi/expr.py @@ -144,30 +144,26 @@ def check_flags(expr: _JSONObject, info: QAPISourceInfo) -> None: def check_if(expr: _JSONObject, info: QAPISourceInfo, source: str) -> None: - def check_if_str(ifcond: object) -> None: - if not isinstance(ifcond, str): - raise QAPISemError( - info, - "'if' condition of %s must be a string or a list of strings" - % source) - if ifcond.strip() == '': - raise QAPISemError( - info, - "'if' condition '%s' of %s makes no sense" - % (ifcond, source)) - ifcond = expr.get('if') if ifcond is None: return + if isinstance(ifcond, list): - if ifcond == []: + if not ifcond: raise QAPISemError( - info, "'if' condition [] of %s is useless" % source) - for elt in ifcond: - check_if_str(elt) + info, f"'if' condition [] of {source} is useless") else: - check_if_str(ifcond) - expr['if'] = [ifcond] + # Normalize to a list + ifcond = expr['if'] = [ifcond] + + for elt in ifcond: + if not isinstance(elt, str): + raise QAPISemError(info, ( + f"'if' condition of {source}" + " must be a string or a list of strings")) + if not elt.strip(): + raise QAPISemError( + info, f"'if' condition '{elt}' of {source} makes no sense") def normalize_members(members: object) -> None:
This is a small rewrite to address some minor style nits. Don't compare against the empty list to check for the empty condition, and move the normalization forward to unify the check on the now-normalized structure. With the check unified, the local nested function isn't needed anymore and can be brought down into the normal flow of the function. With the nesting level changed, shuffle the error strings around a bit to get them to fit in 79 columns. Note: although ifcond is typed as Sequence[str] elsewhere, we *know* that the parser will produce real, bona-fide lists. It's okay to check isinstance(ifcond, list) here. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/expr.py | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-)