John Snow <js...@redhat.com> writes: > On Tue, Nov 21, 2023, 8:33 AM Markus Armbruster <arm...@redhat.com> wrote: > >> John Snow <js...@redhat.com> writes: >> >> > It simplifies typing to mandate that entities will always have a name; >> > to achieve this we can occasionally assign an internal name. This >> > alleviates errors such as: >> > >> > qapi/schema.py:287: error: Argument 1 to "__init__" of >> > "QAPISchemaEntity" has incompatible type "None"; expected "str" >> > [arg-type] >> > >> > Trying to fix it the other way by allowing entities to only have >> > optional names opens up a nightmare portal of whackamole to try and >> > audit that every other pathway doesn't actually pass a None name when we >> > expect it to; this is the simpler direction of consitifying the typing. >> >> Arguably, that nightmare is compile-time proof of "we are not mistaking >> QAPISchemaInclude for a named entity". >> >> When I added the include directive, I shoehorned it into the existing >> representation of the QAPI schema as "list of QAPISchemaEntity" by >> making it a subtype of QAPISchemaEntity. That was a somewhat lazy hack. >> >> Note that qapi-code-gen.rst distinguishes between definitions and >> directives. >> >> The places where mypy gripes that .name isn't 'str' generally want >> something with a name (what qapi-code-gen.rst calls a definition). If >> we somehow pass them an include directive, they'll use None for a name, >> which is no good. mypy is pointing out this problem. >> >> What to do about it? >> >> 1. Paper it over: give include directives some made-up name (this >> patch). Now the places use the made-up name instead of None, and mypy >> can't see the problem anymore. >> >> 2. Assert .name is not None until mypy is happy. I guess that's what >> you called opening "a nightmare portal of whackamole". >> > > Yep. > > >> 3. Clean up the typing: have a type for top-level expression (has no >> name), and a subtype for definition (has a name). Rough sketch >> appended. Thoughts? >> > > Oh, that'll work. I tried to keep to "minimal SLOC" but where you want to > see deeper fixes, I'm happy to deviate. I'll give it a shot.
I do appreciate the minimal fix! I *like* exploring "minimal" first. In this case, the exploration led me to not like my lazy hack anymore :) [...] > I'll try the refactor out in a patch at the end of my series and see how > feasible it is. > > (I haven't reviewed it deeply yet, so if there's an obvious problem I'll > find it when I go to implement this. conceptually it seems fine.) Thanks!
