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

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> 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".

Quite excusable.  We lack the mypy experience to predict such outcomes.

> 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.

I don't worry about bisectability.  The issue is reviewability.

Here's the best case for me reviewing a single patch.  First, the commit
message convinces me this makes sense.  Then I read the patch mostly in
order.  It does what the commit message made me expect, I think I
understand how it does it, and it doesn't touch anything I know to be
subtle.

Here's the best case for me reviewing a patch series: every patch in
order is a best case review.

As soon as review deviates from this best case, I slow down.  A lot.  If
there is something I didn't expect, maybe I'm misunderstanding the
patch's purpose.  If I feel confused about how the patch achieves its
purpose, I better figure it out.  If something subtle is being touched,
I better recall its subtleties and carefully check the patch.  Slow and
exhausting work.

This way of review can be overly careful.  But even deciding "this isn't
important, let it go" is slow, unless I do it wholesale.  All we get
then is "looks good at a glance".  But that's maybe an Acked-by,
certainly not a Reviewed-by.

Me finding the patch where the type hints start to be "serious" is slow.
Me mentally separating changes to type hints from other changes in
patches before that point is slow.  Me examining the type hints at that
point (which need not be entirely visible in the patch) is slow.

If the annotations in the intermediate state don't have to be good, do
they have to be there?  If John can take them out, review will be easier
and faster.