| Message ID | 20190924132830.15835-20-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: Pay back some frontend technical debt | expand |
On 9/24/19 8:28 AM, Markus Armbruster wrote: > Split check_flags() off check_keys() and have check_exprs() call it > later, so its error messages gain an "in definition" line. Tweak the > error messages. > > Checking values in a function named check_keys() is unclean anyway. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > scripts/qapi/common.py | 22 ++++++++++++---------- > tests/qapi-schema/allow-preconfig-test.err | 3 ++- > tests/qapi-schema/args-bad-boxed.err | 3 ++- > tests/qapi-schema/oob-test.err | 3 ++- > tests/qapi-schema/type-bypass-bad-gen.err | 3 ++- > 5 files changed, 20 insertions(+), 14 deletions(-) > > + > +def check_flags(expr, info): > + for key in ['gen', 'success-response']: > + if key in expr and expr[key] is not False: Is it any more pythonic and/or a micro-optimization to compress this to: if expr.get(key, False) is not False: > + raise QAPISemError( > + info, "flag '%s' may only use false value" % key) > + for key in ['boxed', 'allow-oob', 'allow-preconfig']: > + if key in expr and expr[key] is not True: and here too. Reviewed-by: Eric Blake <eblake@redhat.com>
Eric Blake <eblake@redhat.com> writes: > On 9/24/19 8:28 AM, Markus Armbruster wrote: >> Split check_flags() off check_keys() and have check_exprs() call it >> later, so its error messages gain an "in definition" line. Tweak the >> error messages. >> >> Checking values in a function named check_keys() is unclean anyway. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> scripts/qapi/common.py | 22 ++++++++++++---------- >> tests/qapi-schema/allow-preconfig-test.err | 3 ++- >> tests/qapi-schema/args-bad-boxed.err | 3 ++- >> tests/qapi-schema/oob-test.err | 3 ++- >> tests/qapi-schema/type-bypass-bad-gen.err | 3 ++- >> 5 files changed, 20 insertions(+), 14 deletions(-) >> > >> + >> +def check_flags(expr, info): >> + for key in ['gen', 'success-response']: >> + if key in expr and expr[key] is not False: > > Is it any more pythonic and/or a micro-optimization to compress this to: > > if expr.get(key, False) is not False: > >> + raise QAPISemError( >> + info, "flag '%s' may only use false value" % key) >> + for key in ['boxed', 'allow-oob', 'allow-preconfig']: >> + if key in expr and expr[key] is not True: > > and here too. Will do. > Reviewed-by: Eric Blake <eblake@redhat.com> Thanks!
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> writes: > >> On 9/24/19 8:28 AM, Markus Armbruster wrote: >>> Split check_flags() off check_keys() and have check_exprs() call it >>> later, so its error messages gain an "in definition" line. Tweak the >>> error messages. >>> >>> Checking values in a function named check_keys() is unclean anyway. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> --- >>> scripts/qapi/common.py | 22 ++++++++++++---------- >>> tests/qapi-schema/allow-preconfig-test.err | 3 ++- >>> tests/qapi-schema/args-bad-boxed.err | 3 ++- >>> tests/qapi-schema/oob-test.err | 3 ++- >>> tests/qapi-schema/type-bypass-bad-gen.err | 3 ++- >>> 5 files changed, 20 insertions(+), 14 deletions(-) >>> >> >>> + >>> +def check_flags(expr, info): >>> + for key in ['gen', 'success-response']: >>> + if key in expr and expr[key] is not False: >> >> Is it any more pythonic and/or a micro-optimization to compress this to: >> >> if expr.get(key, False) is not False: >> >>> + raise QAPISemError( >>> + info, "flag '%s' may only use false value" % key) >>> + for key in ['boxed', 'allow-oob', 'allow-preconfig']: >>> + if key in expr and expr[key] is not True: >> >> and here too. > > Will do. Second thoughts in the morning: if key in expr and expr[key] is not VALUE: feels slightly clearer than if expr.get(key, VALUE) is not VALUE: >> Reviewed-by: Eric Blake <eblake@redhat.com> > > Thanks!
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index fe99e59153..4bfc007833 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -910,16 +910,17 @@ def check_keys(expr, info, meta, required, optional=[]): required = required + [meta] source = "%s '%s'" % (meta, name) check_known_keys(expr, info, source, required, optional) - for (key, value) in expr.items(): - if key in ['gen', 'success-response'] and value is not False: - raise QAPISemError(info, - "'%s' of %s '%s' should only use false value" - % (key, meta, name)) - if (key in ['boxed', 'allow-oob', 'allow-preconfig'] - and value is not True): - raise QAPISemError(info, - "'%s' of %s '%s' should only use true value" - % (key, meta, name)) + + +def check_flags(expr, info): + for key in ['gen', 'success-response']: + if key in expr and expr[key] is not False: + raise QAPISemError( + info, "flag '%s' may only use false value" % key) + for key in ['boxed', 'allow-oob', 'allow-preconfig']: + if key in expr and expr[key] is not True: + raise QAPISemError( + info, "flag '%s' may only use true value" % key) def normalize_enum(expr): @@ -1022,6 +1023,7 @@ def check_exprs(exprs): assert False, 'unexpected meta type' check_if(expr, info) + check_flags(expr, info) if doc: doc.check_expr(expr) diff --git a/tests/qapi-schema/allow-preconfig-test.err b/tests/qapi-schema/allow-preconfig-test.err index 700d583306..2a4e6ce663 100644 --- a/tests/qapi-schema/allow-preconfig-test.err +++ b/tests/qapi-schema/allow-preconfig-test.err @@ -1 +1,2 @@ -tests/qapi-schema/allow-preconfig-test.json:2: 'allow-preconfig' of command 'allow-preconfig-test' should only use true value +tests/qapi-schema/allow-preconfig-test.json: In command 'allow-preconfig-test': +tests/qapi-schema/allow-preconfig-test.json:2: flag 'allow-preconfig' may only use true value diff --git a/tests/qapi-schema/args-bad-boxed.err b/tests/qapi-schema/args-bad-boxed.err index ad0d417321..31d39038fc 100644 --- a/tests/qapi-schema/args-bad-boxed.err +++ b/tests/qapi-schema/args-bad-boxed.err @@ -1 +1,2 @@ -tests/qapi-schema/args-bad-boxed.json:2: 'boxed' of command 'foo' should only use true value +tests/qapi-schema/args-bad-boxed.json: In command 'foo': +tests/qapi-schema/args-bad-boxed.json:2: flag 'boxed' may only use true value diff --git a/tests/qapi-schema/oob-test.err b/tests/qapi-schema/oob-test.err index 35b60f7480..3c2ba6e0fd 100644 --- a/tests/qapi-schema/oob-test.err +++ b/tests/qapi-schema/oob-test.err @@ -1 +1,2 @@ -tests/qapi-schema/oob-test.json:2: 'allow-oob' of command 'oob-command-1' should only use true value +tests/qapi-schema/oob-test.json: In command 'oob-command-1': +tests/qapi-schema/oob-test.json:2: flag 'allow-oob' may only use true value diff --git a/tests/qapi-schema/type-bypass-bad-gen.err b/tests/qapi-schema/type-bypass-bad-gen.err index a83c3c655d..1077651896 100644 --- a/tests/qapi-schema/type-bypass-bad-gen.err +++ b/tests/qapi-schema/type-bypass-bad-gen.err @@ -1 +1,2 @@ -tests/qapi-schema/type-bypass-bad-gen.json:2: 'gen' of command 'foo' should only use false value +tests/qapi-schema/type-bypass-bad-gen.json: In command 'foo': +tests/qapi-schema/type-bypass-bad-gen.json:2: flag 'gen' may only use false value
Split check_flags() off check_keys() and have check_exprs() call it later, so its error messages gain an "in definition" line. Tweak the error messages. Checking values in a function named check_keys() is unclean anyway. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- scripts/qapi/common.py | 22 ++++++++++++---------- tests/qapi-schema/allow-preconfig-test.err | 3 ++- tests/qapi-schema/args-bad-boxed.err | 3 ++- tests/qapi-schema/oob-test.err | 3 ++- tests/qapi-schema/type-bypass-bad-gen.err | 3 ++- 5 files changed, 20 insertions(+), 14 deletions(-)