Eric Blake <ebl...@redhat.com> writes: > On 09/23/2014 08:56 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> writes: >> >>> The previous commit demonstrated that the generator overlooked some >>> fairly basic broken expressions: >>> - missing metataype >>> - metatype key has a non-string value >>> - unknown key in relation to the metatype >>> - conflicting metatype (this patch treats the second metatype as an >>> unknown key of the first key visited, which is not necessarily the >>> first key the user typed) >> >> The whole thing is a Saturday afternoon hack, with extra hacks bolted on >> left & right. > > And this series is a sequence of MY Saturday afternoon hacks in cleaning > it up :)
Much appreciated! >>> Add check_keys to cover these situations, and update testcases to >>> match. A couple other tests (enum-missing-data, indented-expr) had >>> to change since the validation added here occurs so early. >>> >>> While valid .json files won't trigger any of these cases, we might >>> as well be nicer to developers that make a typo while trying to add >>> new QAPI code. >>> >>> Signed-off-by: Eric Blake <ebl...@redhat.com> >> > >>> +def check_keys(expr_elem, meta, required, optional=[]): >>> + expr = expr_elem['expr'] >>> + info = expr_elem['info'] >>> + name = expr[meta] >> >> Caller ensures expr[meta] exists. Okay. >> >>> + if not isinstance(name, basestring): >> >> I'm a Python noob: why basestring and not str? > > Me too. No clue. Copy and paste from existing code. > http://git.qemu.org/?p=qemu.git;a=blob;f=scripts/qapi.py;h=77d46aa;hb=769188d3b#l361 Yes. It's our only use of basestring. Other places use isinstance(FOO, str). The basestring use comes from Kevin's commit b35284e. Kevin, why basestring and not str? >> >>> + raise QAPIExprError(info, >>> + "%s key must have a string value" % meta) >>> + expr_elem['name'] = name >> >> Where is this used? > > Hmm, I know I used it at one point in my series (to be able to print the > name of an expression in check_type added in 13/19, without having to > repeat the dance of if enum: expr['enum'] elif union: expr['union'] etc. > in multiple places). Although in my refactoring, I may have eliminated > the need for it after all. If it's not in use at the end of the series, > I can drop it. A quick grep comes up empty. >>> + required.append(meta) >> >> Ugly side effect. To avoid, either make a new list here >> >> required = required + [ meta ] >> >> or do nothing here and... >> >>> + for (key, value) in expr.items(): >>> + if not key in required and not key in optional: >> >> ... add "and key != meta" to this condition. > > Shows my inexperience in python. Sure, I can fix. Looks like I get to > spin a v5. > >> >> This hunk is easier to review with whitespace ignored: > > Indeed. Alas, python is a stickler about whitespace reindentation. > Since I have to respin, I'll probably split into two pieces (one with a > no-op change that reindents, the other for the improvements). Not necessary for me, I'm comfy with telling Emacs to hide whitespace differences :)