Re: [PATCH 13/19] qapi/schema: fix typing for QAPISchemaVariants.tag_member

[John Snow]

On Wed, Nov 22, 2023 at 11:02 AM John Snow <jsnow@redhat.com> wrote:
>
> On Wed, Nov 22, 2023 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote:
> >
> > John Snow <jsnow@redhat.com> writes:
> >
> > > There are two related changes here:
> > >
> > > (1) We need to perform type narrowing for resolving the type of
> > >     tag_member during check(), and
> > >
> > > (2) tag_member is a delayed initialization field, but we can hide it
> > >     behind a property that raises an Exception if it's called too
> > >     early. This simplifies the typing in quite a few places and avoids
> > >     needing to assert that the "tag_member is not None" at a dozen
> > >     callsites, which can be confusing and suggest the wrong thing to a
> > >     drive-by contributor.
> > >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > Without looking closely: review of PATCH 10 applies, doesn't it?
> >
>
> Yep!

Hm, actually, maybe not quite as cleanly.

The problem is we *are* initializing that field immediately with
whatever we were passed in during __init__, which means the field is
indeed Optional. Later, during check(), we happen to eliminate that
usage of None.

To remove the use of the @property trick here, we could:

... declare the field, then only initialize it if we were passed a
non-None value. But then check() would need to rely on something like
hasattr to check if it was set or not, which is maybe an unfortunate
code smell.
So I think you'd still wind up needing a ._tag_member field which is
Optional and always gets set during __init__, then setting a proper
.tag_member field during check().

Or I could just leave this one as-is. Or something else. I think the
dirt has to get swept somewhere, because we don't *always* have enough
information to fully initialize it at __init__ time, it's a
conditional delayed initialization, unlike the others which are
unconditionally delayed.

--js