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 :) > >> 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 > >> + 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. > >> + 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). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
