| Message ID | 20231116014350.653792-9-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qapi: statically type schema.py | expand |
John Snow <jsnow@redhat.com> writes: > This function is a bit hard to type as-is; mypy needs some assertions to > assist with the type narrowing. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/schema.py | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > index a1094283828..3308f334872 100644 > --- a/scripts/qapi/schema.py > +++ b/scripts/qapi/schema.py > @@ -968,8 +968,12 @@ def lookup_entity(self, name, typ=None): > return None > return ent > > - def lookup_type(self, name): > - return self.lookup_entity(name, QAPISchemaType) > + def lookup_type(self, name: str) -> Optional[QAPISchemaType]: Any particular reason not to delay the type hints until PATCH 16? > + typ = self.lookup_entity(name, QAPISchemaType) > + if typ is None: > + return None > + assert isinstance(typ, QAPISchemaType) > + return typ Would typ = self.lookup_entity(name, QAPISchemaType) assert isinstance(typ, Optional[QAPISchemaType]) return typ work? > > def resolve_type(self, name, info, what): > typ = self.lookup_type(name)
On Tue, Nov 21, 2023, 9:21 AM Markus Armbruster <armbru@redhat.com> wrote: > John Snow <jsnow@redhat.com> writes: > > > This function is a bit hard to type as-is; mypy needs some assertions to > > assist with the type narrowing. > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > --- > > scripts/qapi/schema.py | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > > index a1094283828..3308f334872 100644 > > --- a/scripts/qapi/schema.py > > +++ b/scripts/qapi/schema.py > > @@ -968,8 +968,12 @@ def lookup_entity(self, name, typ=None): > > return None > > return ent > > > > - def lookup_type(self, name): > > - return self.lookup_entity(name, QAPISchemaType) > > + def lookup_type(self, name: str) -> Optional[QAPISchemaType]: > > Any particular reason not to delay the type hints until PATCH 16? > I forget. In some cases I did things a little differently so that the type checking would pass for each patch in the series, which sometimes required some concessions. Is this one of those cases? Uh, I forget. If it isn't, its almost certainly the case that I just figured I'd type this one function in one place instead of splitting it apart into two patches. I can try to shift the typing later and see what happens if you prefer it that way. > > + typ = self.lookup_entity(name, QAPISchemaType) > > + if typ is None: > > + return None > > + assert isinstance(typ, QAPISchemaType) > > + return typ > > Would > > typ = self.lookup_entity(name, QAPISchemaType) > assert isinstance(typ, Optional[QAPISchemaType]) > return typ > > work? > I don't *think* so, Optional isn't a runtime construct. We can combine it into "assert x is None or isinstance(x, foo)" though - I believe that's used elsewhere in the qapi generator. > > > > def resolve_type(self, name, info, what): > > typ = self.lookup_type(name) > >
John Snow <jsnow@redhat.com> writes: > On Tue, Nov 21, 2023, 9:21 AM Markus Armbruster <armbru@redhat.com> wrote: > >> John Snow <jsnow@redhat.com> writes: >> >> > This function is a bit hard to type as-is; mypy needs some assertions to >> > assist with the type narrowing. >> > >> > Signed-off-by: John Snow <jsnow@redhat.com> >> > --- >> > scripts/qapi/schema.py | 8 ++++++-- >> > 1 file changed, 6 insertions(+), 2 deletions(-) >> > >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py >> > index a1094283828..3308f334872 100644 >> > --- a/scripts/qapi/schema.py >> > +++ b/scripts/qapi/schema.py >> > @@ -968,8 +968,12 @@ def lookup_entity(self, name, typ=None): >> > return None >> > return ent >> > >> > - def lookup_type(self, name): >> > - return self.lookup_entity(name, QAPISchemaType) >> > + def lookup_type(self, name: str) -> Optional[QAPISchemaType]: >> >> Any particular reason not to delay the type hints until PATCH 16? >> > > I forget. In some cases I did things a little differently so that the type > checking would pass for each patch in the series, which sometimes required > some concessions. > > Is this one of those cases? Uh, I forget. > > If it isn't, its almost certainly the case that I just figured I'd type > this one function in one place instead of splitting it apart into two > patches. > > I can try to shift the typing later and see what happens if you prefer it > that way. Well, you structured the series as "minor code changes to prepare for type hints", followed by "add type hints". So that's what I expect. When patches deviate from what I expect, I go "am I missing something?" Adding type hints along the way could work, too. But let's try to stick to the plan, and add them all in PATCH 16. >> > + typ = self.lookup_entity(name, QAPISchemaType) >> > + if typ is None: >> > + return None >> > + assert isinstance(typ, QAPISchemaType) >> > + return typ >> >> Would >> >> typ = self.lookup_entity(name, QAPISchemaType) >> assert isinstance(typ, Optional[QAPISchemaType]) >> return typ >> >> work? >> > > I don't *think* so, Optional isn't a runtime construct. Let me try... $ python Python 3.11.5 (main, Aug 28 2023, 00:00:00) [GCC 12.3.1 20230508 (Red Hat 12.3.1-1)] on linux Type "help", "copyright", "credits" or "license" for more information. >>> from typing import Optional >>> x=None >>> isinstance(x, Optional[str]) True >>> > We can combine it > into "assert x is None or isinstance(x, foo)" though - I believe that's > used elsewhere in the qapi generator. >> > >> > def resolve_type(self, name, info, what): >> > typ = self.lookup_type(name) >> >>
On Wed, Nov 22, 2023 at 7:09 AM Markus Armbruster <armbru@redhat.com> wrote: > > John Snow <jsnow@redhat.com> writes: > > > On Tue, Nov 21, 2023, 9:21 AM Markus Armbruster <armbru@redhat.com> wrote: > > > >> John Snow <jsnow@redhat.com> writes: > >> > >> > This function is a bit hard to type as-is; mypy needs some assertions to > >> > assist with the type narrowing. > >> > > >> > Signed-off-by: John Snow <jsnow@redhat.com> > >> > --- > >> > scripts/qapi/schema.py | 8 ++++++-- > >> > 1 file changed, 6 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py > >> > index a1094283828..3308f334872 100644 > >> > --- a/scripts/qapi/schema.py > >> > +++ b/scripts/qapi/schema.py > >> > @@ -968,8 +968,12 @@ def lookup_entity(self, name, typ=None): > >> > return None > >> > return ent > >> > > >> > - def lookup_type(self, name): > >> > - return self.lookup_entity(name, QAPISchemaType) > >> > + def lookup_type(self, name: str) -> Optional[QAPISchemaType]: > >> > >> Any particular reason not to delay the type hints until PATCH 16? > >> > > > > I forget. In some cases I did things a little differently so that the type > > checking would pass for each patch in the series, which sometimes required > > some concessions. > > > > Is this one of those cases? Uh, I forget. > > > > If it isn't, its almost certainly the case that I just figured I'd type > > this one function in one place instead of splitting it apart into two > > patches. > > > > I can try to shift the typing later and see what happens if you prefer it > > that way. > > Well, you structured the series as "minor code changes to prepare for > type hints", followed by "add type hints". So that's what I expect. > When patches deviate from what I expect, I go "am I missing something?" > > Adding type hints along the way could work, too. But let's try to stick > to the plan, and add them all in PATCH 16. > > >> > + typ = self.lookup_entity(name, QAPISchemaType) > >> > + if typ is None: > >> > + return None > >> > + assert isinstance(typ, QAPISchemaType) > >> > + return typ > >> > >> Would > >> > >> typ = self.lookup_entity(name, QAPISchemaType) > >> assert isinstance(typ, Optional[QAPISchemaType]) > >> return typ > >> > >> work? > >> > > > > I don't *think* so, Optional isn't a runtime construct. > > Let me try... > > $ python > Python 3.11.5 (main, Aug 28 2023, 00:00:00) [GCC 12.3.1 20230508 (Red Hat 12.3.1-1)] on linux > Type "help", "copyright", "credits" or "license" for more information. > >>> from typing import Optional > >>> x=None > >>> isinstance(x, Optional[str]) > True > >>> Huh. I ... huh! Well, this apparently only works in Python 3.10!+ TypeError: Subscripted generics cannot be used with class and instance checks > > > We can combine it > > into "assert x is None or isinstance(x, foo)" though - I believe that's > > used elsewhere in the qapi generator. > > >> > > >> > def resolve_type(self, name, info, what): > >> > typ = self.lookup_type(name) > >> > >> >
John Snow <jsnow@redhat.com> writes: > On Wed, Nov 22, 2023 at 7:09 AM Markus Armbruster <armbru@redhat.com> wrote: >> >> John Snow <jsnow@redhat.com> writes: >> >> > On Tue, Nov 21, 2023, 9:21 AM Markus Armbruster <armbru@redhat.com> wrote: >> > >> >> John Snow <jsnow@redhat.com> writes: >> >> >> >> > This function is a bit hard to type as-is; mypy needs some assertions to >> >> > assist with the type narrowing. >> >> > >> >> > Signed-off-by: John Snow <jsnow@redhat.com> >> >> > --- >> >> > scripts/qapi/schema.py | 8 ++++++-- >> >> > 1 file changed, 6 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py >> >> > index a1094283828..3308f334872 100644 >> >> > --- a/scripts/qapi/schema.py >> >> > +++ b/scripts/qapi/schema.py >> >> > @@ -968,8 +968,12 @@ def lookup_entity(self, name, typ=None): >> >> > return None >> >> > return ent >> >> > >> >> > - def lookup_type(self, name): >> >> > - return self.lookup_entity(name, QAPISchemaType) >> >> > + def lookup_type(self, name: str) -> Optional[QAPISchemaType]: [...] >> >> > + typ = self.lookup_entity(name, QAPISchemaType) >> >> > + if typ is None: >> >> > + return None >> >> > + assert isinstance(typ, QAPISchemaType) >> >> > + return typ >> >> >> >> Would >> >> >> >> typ = self.lookup_entity(name, QAPISchemaType) >> >> assert isinstance(typ, Optional[QAPISchemaType]) >> >> return typ >> >> >> >> work? >> >> >> > >> > I don't *think* so, Optional isn't a runtime construct. >> >> Let me try... >> >> $ python >> Python 3.11.5 (main, Aug 28 2023, 00:00:00) [GCC 12.3.1 20230508 (Red Hat 12.3.1-1)] on linux >> Type "help", "copyright", "credits" or "license" for more information. >> >>> from typing import Optional >> >>> x=None >> >>> isinstance(x, Optional[str]) >> True >> >>> > > Huh. I ... huh! > > Well, this apparently only works in Python 3.10!+ > > TypeError: Subscripted generics cannot be used with class and instance checks We should be able to use it "soon" after 3.9 reaches EOL, approximately October 2025. *Sigh* >> >> > We can combine it >> > into "assert x is None or isinstance(x, foo)" though - I believe that's >> > used elsewhere in the qapi generator. >> >> >> > >> >> > def resolve_type(self, name, info, what): >> >> > typ = self.lookup_type(name) >> >> >> >> >>
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py index a1094283828..3308f334872 100644 --- a/scripts/qapi/schema.py +++ b/scripts/qapi/schema.py @@ -968,8 +968,12 @@ def lookup_entity(self, name, typ=None): return None return ent - def lookup_type(self, name): - return self.lookup_entity(name, QAPISchemaType) + def lookup_type(self, name: str) -> Optional[QAPISchemaType]: + typ = self.lookup_entity(name, QAPISchemaType) + if typ is None: + return None + assert isinstance(typ, QAPISchemaType) + return typ def resolve_type(self, name, info, what): typ = self.lookup_type(name)
This function is a bit hard to type as-is; mypy needs some assertions to assist with the type narrowing. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/schema.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)