Re: [PATCH v2 05/11] qapi/introspect.py: add preliminary type hint annotations

[Eduardo Habkost <ehabkost@redhat.com>]

On Wed, Dec 16, 2020 at 08:51:10AM +0100, Markus Armbruster wrote:
[...]
> You guys clearly struggled with the tree data structure.  Documentation
> would have helped[*].  Since you're going to replace it (PATCH 09),
> adding it now makes little sense.
> 
> *My* struggle is with the type annotations.
> 
> The initial state is messy to type, in part due to mypy's surprising
> inability to deal with recursive types, in part because the tree data
> structure is messier than it could be.
> 
> Much of the series is cleanup that benefits the typing.  Makes sense.
> 
> What makes review hard for me: you add (fairly complicated) typing
> first, then evolve it along with the cleanups.  I have to first grok the
> complicated typing (a struggle), then for each cleanup grok the type
> changes in addition to the code changes.
> 
> I believe adding the typing before the cleanups is a mistake.

Possibly my fault, as I remember asking John to do that (in
earlier versions of these patches, type annotations were added
after cleanup).

> 
> I share the desire to have type annotations that help with understanding
> the code.  I understand the desire to have them sooner rather than
> later.  I just think they're a costly distraction at this stage for this
> code.  Once you understand the data structure, the cleanups are fairly
> straightforward.
> 

I expected the type annotations to be a simple and helpful tool
for understanding the data structure before refactoring.  In the
case of introspect.py, I was completely wrong about "simple", and
I'm not entirely sure about "helpful".

I wasn't expecting them to be an obstacle for patch review,
though.  If the type annotations look good at the end of the
series, do we care about the intermediate state?  Bisectability
isn't an issue because type annotations are ignored by the Python
interpreter.

-- 
Eduardo