[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 08/14] qapi/introspect.py: create a typed 'Annotated' data

From: John Snow
Subject: Re: [PATCH v4 08/14] qapi/introspect.py: create a typed 'Annotated' data strutcure
Date: Thu, 4 Feb 2021 11:20:48 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

On 2/4/21 10:37 AM, Markus Armbruster wrote:
Eduardo Habkost <ehabkost@redhat.com> writes:

On Wed, Feb 03, 2021 at 03:47:36PM +0100, Markus Armbruster wrote:
John Snow <jsnow@redhat.com> writes:

Presently, we use a tuple to attach a dict containing annotations
(comments and compile-time conditionals) to a tree node. This is
undesirable because dicts are difficult to strongly type; promoting it
to a real class allows us to name the values and types of the
annotations we are expecting.

In terms of typing, the Annotated<T> type serves as a generic container
where the annotated node's type is preserved, allowing for greater
specificity than we'd be able to provide without a generic.

Signed-off-by: John Snow <jsnow@redhat.com>
+class Annotated(Generic[_NodeT]):
+    """
+    Annotated generally contains a SchemaInfo-like type (as a dict),
+    But it also used to wrap comments/ifconds around scalar leaf values,
+    for the benefit of features and enums.
+    """
+    # Remove after 3.7 adds @dataclass:

Make this

        # TODO Remove after Python 3.7 ...

to give us a fighting chance to remember.

+    # pylint: disable=too-few-public-methods
+    def __init__(self, value: _NodeT, ifcond: Iterable[str],
+                 comment: Optional[str] = None):

Why not simply value: _value?

   x = C(1)
   y: C[int]
   y = C('x')  # mistake

Declaring value as _NodeT does:
- Make the inferred type of x be Annotated[int].
- Catch the mistake above.

I smell overengineering.  I may well be wrong.

Without doubt, there are uses for using the type system for keeping
SomeGenericType[SomeType] and SomeGenericType[AnotherType] apart.

But what do we gain by keeping the Annotated[T] for the possible T

_tree_to_qlit() doesn't care: it peels off the wrapper holding ifcond
and comment, and recurses for the JSON so wrapped.  Regardless of what
was wrapped, i.e. what kind of T we got.

Heck, it works just fine even if you wrap your JSON multiple times.  It
doesn't give a hoot whether that makes sense.  Making sense is the
caller's business.

So what does care?

Or am I simply confused?

PS: As far as I can tell, _tree_to_qlit() doesn't give a hoot whether a
dictionary's values are wrapped, either.

You're right that it offers no necessary power to the automated checking, I cede as much in my other replies to you.

(1) I hope the explanation was helpful, because the technique will return for structures like QAPISchemaMember where it does become more obviously useful to disambiguate the wrapped types.

(2) It still has a notational benefit to the human for documentation and IDE purposes, e.g. at the end of this series:

self._trees: List[Annotated[SchemaInfo]] = []

That's nice! It tells us exactly what's in there.

Does it work if I dumb it down to just:

self.trees: List[AnnotatedThingy] = []

Yes, but it's worse to the human and the IDE both. I consider type hints as serving a dual purpose; both provability and declaration of intent.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]