On Wed, Nov 22, 2023 at 11:02 AM John Snow <js...@redhat.com> wrote: > > On Wed, Nov 22, 2023 at 9:05 AM Markus Armbruster <arm...@redhat.com> wrote: > > > > John Snow <js...@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 <js...@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
