On Wed, Nov 22, 2023 at 4:38 AM Markus Armbruster <arm...@redhat.com> wrote: > > 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! >
Got this working with minor changes to sphinx's qapidox extension, which isn't typechecked so it escaped notice. I'm going to continue working through your feedback as-is, then when I get to the end of the series, I'll attempt to float this patch forward to replace this patch under discussion. Thanks! --js
