Eric Blake <ebl...@redhat.com> writes:
> On 09/03/2015 08:29 AM, Markus Armbruster wrote:
>> The QAPI code generators work with a syntax tree (nested dictionaries)
>> plus a few symbol tables (also dictionaries) on the side.
>>
>
>> +
>> +class QAPISchemaArrayType(QAPISchemaType):
>> + def __init__(self, name, info, element_type):
>> + QAPISchemaType.__init__(self, name, info)
>> + assert isinstance(element_type, str)
>> + self.element_type_name = element_type
>> + self.element_type = None
>
> Would it make sense to have:
>
> self._element_type_name = element_type
> self.element_type = None
>
>> + def check(self, schema):
>> + self.element_type = schema.lookup_type(self.element_type_name)
>> + assert self.element_type
>> +
>
> and the corresponding
> self.element_type = schema.lookup_type(self._element_type_name)
>
> to make it obvious that ._element_type_name is for internal use only,
> while .element_type is the preferred member for queries/manipulation by
> qapi-*.py, which should only be using this object after check() has been
> run?
>
>> +class QAPISchemaObjectType(QAPISchemaType):
>> + def __init__(self, name, info, base, local_members, variants):
>> + QAPISchemaType.__init__(self, name, info)
>> + assert base == None or isinstance(base, str)
>> + for m in local_members:
>> + assert isinstance(m, QAPISchemaObjectTypeMember)
>> + assert variants == None \
>> + or isinstance(variants, QAPISchemaObjectTypeVariants)
>> + self.base_name = base
>> + self.base = None
>> + self.local_members = local_members
>> + self.variants = variants
>> + self.members = None
>
> Similarly for self._base_name
>
>> +
>> +class QAPISchemaObjectTypeMember(object):
>> + def __init__(self, name, typ, optional):
>> + assert isinstance(name, str)
>> + assert isinstance(typ, str)
>> + assert isinstance(optional, bool)
>> + self.name = name
>> + self.type_name = typ
>> + self.type = None
>> + self.optional = optional
>
> and for self._type_name
>
>> +class QAPISchemaObjectTypeVariants(object):
>> + def __init__(self, tag_name, tag_enum, variants):
>> + assert tag_name == None or isinstance(tag_name, str)
>> + assert tag_enum == None or isinstance(tag_enum, str)
>> + for v in variants:
>> + assert isinstance(v, QAPISchemaObjectTypeVariant)
>> + self.tag_name = tag_name
>> + if tag_name:
>> + assert not tag_enum
>> + self.tag_member = None
>> + else:
>> + self.tag_member = QAPISchemaObjectTypeMember('kind', tag_enum,
>> + False)
>
> and eventually for self._tag_name (except that we MUST expose
> self._tag_name until after we fix the C struct to use 'type' instead of
> 'kind').
Python lets you use variants._tag_name. The leading underscore is
advisory (except for wildcard imports, which don't apply here).
>> +class QAPISchemaCommand(QAPISchemaEntity):
>> + def __init__(self, name, info, arg_type, ret_type, gen,
>> success_response):
>> + QAPISchemaEntity.__init__(self, name, info)
>> + assert not arg_type or isinstance(arg_type, str)
>> + assert not ret_type or isinstance(ret_type, str)
>> + self.arg_type_name = arg_type
>> + self.arg_type = None
>> + self.ret_type_name = ret_type
>> + self.ret_type = None
>> + self.gen = gen
>> + self.success_response = success_response
>
> and for self._art_type_name and self._ret_type_name
>
>> +
>> +class QAPISchemaEvent(QAPISchemaEntity):
>> + def __init__(self, name, info, arg_type):
>> + QAPISchemaEntity.__init__(self, name, info)
>> + assert not arg_type or isinstance(arg_type, str)
>> + self.arg_type_name = arg_type
>> + self.arg_type = None
>
> and for self._arg_type_name
>
> Basically, if I'm understanding the python conventions correctly, naming
> an instance member with leading underscore implies that it is for
> private use only; and if nothing else, the rename proves whether any of
> the other python files are relying on a leaky abstraction.
I followed that convention for methods, but didn't bother for instance
variables. Perhaps I should.
