On Wed, Sep 30, 2020 at 02:58:04PM -0400, John Snow wrote: > On 9/30/20 2:39 PM, Eduardo Habkost wrote: > > On Wed, Sep 30, 2020 at 12:31:45AM -0400, John Snow wrote: > > > This replaces _make_tree with Node.__init__(), effectively. By creating > > > it as a generic container, we can more accurately describe the exact > > > nature of this particular Node. > > > > > > Signed-off-by: John Snow <js...@redhat.com> > > > --- > > > scripts/qapi/introspect.py | 77 +++++++++++++++++++------------------- > > > 1 file changed, 38 insertions(+), 39 deletions(-) > > > > > > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py > > > index 43b6ba5df1f..86286e755ca 100644 > > > --- a/scripts/qapi/introspect.py > > > +++ b/scripts/qapi/introspect.py > > > @@ -12,11 +12,12 @@ > > > from typing import ( > > > Dict, > > > + Generic, > > > + Iterable, > > > List, > > > Optional, > > > Sequence, > > > - Tuple, > > > - Union, > > > + TypeVar, > > > ) > > > from .common import ( > > > @@ -43,42 +44,42 @@ > > > # The correct type for TreeNode is actually: > > > -# Union[AnnotatedNode, List[TreeNode], Dict[str, TreeNode], str, bool] > > > +# Union[Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool] > > > # but mypy does not support recursive types yet. > > > TreeNode = object > > > +_NodeType = TypeVar('_NodeType', bound=TreeNode) > > > _DObject = Dict[str, object] > > > -Extra = Dict[str, object] > > > -AnnotatedNode = Tuple[TreeNode, Extra] > > > > Do you have plans to make Node replace TreeNode completely? > > > > I'd understand this patch as a means to reach that goal, but I'm > > not sure the intermediate state of having both Node and TreeNode > > types (that can be confused with each other) is desirable. > > > > The problem is that _tree_to_qlit still accepts a broad array of types. The > "TreeNode" comment above explains that those types are: > > Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool > > Three are containers, two are leaf values. > of the containers, the Node container is special in that it houses > explicitly one of the four other types (but never itself.) > > Even if I somehow always enforced Node[T] heading into _tree_to_qlit, I > would still need to describe what 'T' is, which is another recursive type > that I cannot exactly describe with mypy's current descriptive power: > > INNER_TYPE = List[Node[INNER_TYPE]], Dict[str, Node[INNER_TYPE]], str, bool > > And that's not really a huge win, or indeed any different to the existing > TreeNode being an "object". > > So ... no, I felt like I was going to stop here, where we have > fundamentally: > > 1. Undecorated nodes (list, dict, str, bool) ("TreeNode") > 2. Decorated nodes (Node[T]) ("Node") > > which leads to the question: Why bother swapping Tuple for Node at that > point? > > My answer is simply that having a strong type name allows us to distinguish > this from garden-variety Tuples that might sneak in for other reasons in > other data types.
Would: AnnotatedNode = NewType('AnnotatedNode', Tuple[TreeNode, Extra]) be enough, then? > > Maybe we want a different nomenclature though, like Node vs AnnotatedNode? Yes, I believe having a more explicit name would be better. > > --js > > (Also: 'TreeNode' is just an alias for object, it doesn't mean anything > grammatically. I could just as soon erase it entirely if you felt it > provided no value. It doesn't enforce that it only takes objects we declared > were 'TreeNode' types, for instance. It's just a preprocessor name, > basically.) > > > > -def _make_tree(obj: Union[_DObject, str], ifcond: List[str], > > > - comment: Optional[str] = None) -> AnnotatedNode: > > > - extra: Extra = { > > > - 'if': ifcond, > > > - 'comment': comment, > > > - } > > > - return (obj, extra) > > > +class Node(Generic[_NodeType]): > > > + """ > > > + Node generally contains a SchemaInfo-like type (as a dict), > > > + But it also used to wrap comments/ifconds around leaf value types. > > > + """ > > > + # Remove after 3.7 adds @dataclass: > > > + # pylint: disable=too-few-public-methods > > > + def __init__(self, data: _NodeType, ifcond: Iterable[str], > > > + comment: Optional[str] = None): > > > + self.data = data > > > + self.comment: Optional[str] = comment > > > + self.ifcond: Sequence[str] = tuple(ifcond) > > > -def _tree_to_qlit(obj: TreeNode, > > > - level: int = 0, > > > +def _tree_to_qlit(obj: TreeNode, level: int = 0, > > > suppress_first_indent: bool = False) -> str: > > > def indent(level: int) -> str: > > > return level * 4 * ' ' > > > - if isinstance(obj, tuple): > > > - ifobj, extra = obj > > > - ifcond = extra.get('if') > > > - comment = extra.get('comment') > > > + if isinstance(obj, Node): > > > ret = '' > > > - if comment: > > > - ret += indent(level) + '/* %s */\n' % comment > > > - if ifcond: > > > - ret += gen_if(ifcond) > > > - ret += _tree_to_qlit(ifobj, level) > > > - if ifcond: > > > - ret += '\n' + gen_endif(ifcond) > > > + if obj.comment: > > > + ret += indent(level) + '/* %s */\n' % obj.comment > > > + if obj.ifcond: > > > + ret += gen_if(obj.ifcond) > > > + ret += _tree_to_qlit(obj.data, level) > > > + if obj.ifcond: > > > + ret += '\n' + gen_endif(obj.ifcond) > > > return ret > > > ret = '' > > > @@ -125,7 +126,7 @@ def __init__(self, prefix: str, unmask: bool): > > > ' * QAPI/QMP schema introspection', __doc__) > > > self._unmask = unmask > > > self._schema: Optional[QAPISchema] = None > > > - self._trees: List[AnnotatedNode] = [] > > > + self._trees: List[Node[_DObject]] = [] > > > self._used_types: List[QAPISchemaType] = [] > > > self._name_map: Dict[str, str] = {} > > > self._genc.add(mcgen(''' > > > @@ -192,9 +193,8 @@ def _use_type(self, typ: QAPISchemaType) -> str: > > > @classmethod > > > def _gen_features(cls, > > > - features: List[QAPISchemaFeature] > > > - ) -> List[AnnotatedNode]: > > > - return [_make_tree(f.name, f.ifcond) for f in features] > > > + features: List[QAPISchemaFeature]) -> > > > List[Node[str]]: > > > + return [Node(f.name, f.ifcond) for f in features] > > > def _gen_tree(self, name: str, mtype: str, obj: _DObject, > > > ifcond: List[str], > > > @@ -210,10 +210,10 @@ def _gen_tree(self, name: str, mtype: str, obj: > > > _DObject, > > > obj['meta-type'] = mtype > > > if features: > > > obj['features'] = self._gen_features(features) > > > - self._trees.append(_make_tree(obj, ifcond, comment)) > > > + self._trees.append(Node(obj, ifcond, comment)) > > > def _gen_member(self, > > > - member: QAPISchemaObjectTypeMember) -> AnnotatedNode: > > > + member: QAPISchemaObjectTypeMember) -> > > > Node[_DObject]: > > > obj: _DObject = { > > > 'name': member.name, > > > 'type': self._use_type(member.type) > > > @@ -222,19 +222,19 @@ def _gen_member(self, > > > obj['default'] = None > > > if member.features: > > > obj['features'] = self._gen_features(member.features) > > > - return _make_tree(obj, member.ifcond) > > > + return Node(obj, member.ifcond) > > > def _gen_variants(self, tag_name: str, > > > variants: List[QAPISchemaVariant]) -> _DObject: > > > return {'tag': tag_name, > > > 'variants': [self._gen_variant(v) for v in variants]} > > > - def _gen_variant(self, variant: QAPISchemaVariant) -> AnnotatedNode: > > > + def _gen_variant(self, variant: QAPISchemaVariant) -> Node[_DObject]: > > > obj: _DObject = { > > > 'case': variant.name, > > > 'type': self._use_type(variant.type) > > > } > > > - return _make_tree(obj, variant.ifcond) > > > + return Node(obj, variant.ifcond) > > > def visit_builtin_type(self, name: str, info: > > > Optional[QAPISourceInfo], > > > json_type: str) -> None: > > > @@ -245,8 +245,7 @@ def visit_enum_type(self, name: str, info: > > > QAPISourceInfo, > > > members: List[QAPISchemaEnumMember], > > > prefix: Optional[str]) -> None: > > > self._gen_tree(name, 'enum', > > > - {'values': [_make_tree(m.name, m.ifcond, None) > > > - for m in members]}, > > > + {'values': [Node(m.name, m.ifcond) for m in > > > members]}, > > > ifcond, features) > > > def visit_array_type(self, name: str, info: > > > Optional[QAPISourceInfo], > > > @@ -274,9 +273,9 @@ def visit_alternate_type(self, name: str, info: > > > QAPISourceInfo, > > > variants: QAPISchemaVariants) -> None: > > > self._gen_tree(name, 'alternate', > > > {'members': [ > > > - _make_tree({'type': self._use_type(m.type)}, > > > - m.ifcond, None) > > > - for m in variants.variants]}, > > > + Node({'type': self._use_type(m.type)}, > > > m.ifcond) > > > + for m in variants.variants > > > + ]}, > > > ifcond, features) > > > def visit_command(self, name: str, info: QAPISourceInfo, ifcond: > > > List[str], > > > -- > > > 2.26.2 > > > > > > -- Eduardo