| Message ID | 20200922210101.4081073-23-jsnow@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qapi: static typing conversion, pt1 | expand |
On Tue, Sep 22, 2020 at 05:00:45PM -0400, John Snow wrote: > Annotations do not change runtime behavior. > This commit *only* adds annotations. > > Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
On Tue, Sep 22, 2020 at 05:00:45PM -0400, John Snow wrote: > Annotations do not change runtime behavior. > This commit *only* adds annotations. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > scripts/qapi/mypy.ini | 5 ----- > scripts/qapi/source.py | 31 ++++++++++++++++++------------- > 2 files changed, 18 insertions(+), 18 deletions(-) > > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini > index 9da1dccef4..43c8bd1973 100644 > --- a/scripts/qapi/mypy.ini > +++ b/scripts/qapi/mypy.ini > @@ -39,11 +39,6 @@ disallow_untyped_defs = False > disallow_incomplete_defs = False > check_untyped_defs = False > > -[mypy-qapi.source] > -disallow_untyped_defs = False > -disallow_incomplete_defs = False > -check_untyped_defs = False > - This is what I meant in my comment in the previous patch. It looks like a mix of commit grannurality styles. Not a blocker though. > [mypy-qapi.types] > disallow_untyped_defs = False > disallow_incomplete_defs = False > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py > index e97b9a8e15..1cc6a5b82d 100644 > --- a/scripts/qapi/source.py > +++ b/scripts/qapi/source.py > @@ -11,37 +11,42 @@ > > import copy > import sys > +from typing import List, Optional, TypeVar > > > class QAPISchemaPragma: > - def __init__(self): > + def __init__(self) -> None: I don't follow the reason for typing this... > # Are documentation comments required? > self.doc_required = False > # Whitelist of commands allowed to return a non-dictionary > - self.returns_whitelist = [] > + self.returns_whitelist: List[str] = [] > # Whitelist of entities allowed to violate case conventions > - self.name_case_whitelist = [] > + self.name_case_whitelist: List[str] = [] > > > class QAPISourceInfo: > - def __init__(self, fname, line, parent): > + T = TypeVar('T', bound='QAPISourceInfo') > + > + def __init__(self: T, fname: str, line: int, parent: Optional[T]): And not this... to tune my review approach, should I assume that this series intends to add complete type hints or not? - Cleber.
On 9/23/20 6:36 PM, Cleber Rosa wrote: > On Tue, Sep 22, 2020 at 05:00:45PM -0400, John Snow wrote: >> Annotations do not change runtime behavior. >> This commit *only* adds annotations. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> scripts/qapi/mypy.ini | 5 ----- >> scripts/qapi/source.py | 31 ++++++++++++++++++------------- >> 2 files changed, 18 insertions(+), 18 deletions(-) >> >> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini >> index 9da1dccef4..43c8bd1973 100644 >> --- a/scripts/qapi/mypy.ini >> +++ b/scripts/qapi/mypy.ini >> @@ -39,11 +39,6 @@ disallow_untyped_defs = False >> disallow_incomplete_defs = False >> check_untyped_defs = False >> >> -[mypy-qapi.source] >> -disallow_untyped_defs = False >> -disallow_incomplete_defs = False >> -check_untyped_defs = False >> - > > This is what I meant in my comment in the previous patch. It looks > like a mix of commit grannurality styles. Not a blocker though. > Yep. Just how the chips fell. Some files were just very quick to cleanup and I didn't have to refactor them much when I split things out, so the enablements got rolled in. I will, once reviews are in (and there is a commitment to merge), try to squash things where it seems appropriate. >> [mypy-qapi.types] >> disallow_untyped_defs = False >> disallow_incomplete_defs = False >> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py >> index e97b9a8e15..1cc6a5b82d 100644 >> --- a/scripts/qapi/source.py >> +++ b/scripts/qapi/source.py >> @@ -11,37 +11,42 @@ >> >> import copy >> import sys >> +from typing import List, Optional, TypeVar >> >> >> class QAPISchemaPragma: >> - def __init__(self): >> + def __init__(self) -> None: > > I don't follow the reason for typing this... > >> # Are documentation comments required? >> self.doc_required = False >> # Whitelist of commands allowed to return a non-dictionary >> - self.returns_whitelist = [] >> + self.returns_whitelist: List[str] = [] >> # Whitelist of entities allowed to violate case conventions >> - self.name_case_whitelist = [] >> + self.name_case_whitelist: List[str] = [] >> >> >> class QAPISourceInfo: >> - def __init__(self, fname, line, parent): >> + T = TypeVar('T', bound='QAPISourceInfo') >> + >> + def __init__(self: T, fname: str, line: int, parent: Optional[T]): > > And not this... to tune my review approach, should I assume that this > series intends to add complete type hints or not? > This is a mypy quirk you've discovered that I've simply forgotten about. When __init__ has *no* arguments, you need to annotate its return to explain to mypy that you have fully typed that method. It's a sentinel that says "Please type check this class!" When __init__ has some arguments, you only need to type those arguments and not the return type. The sentinel is not needed. __init__ *never* returns anything, so I opted to omit this useless annotation whenever it was possible to do so. > - Cleber. >
John Snow <jsnow@redhat.com> writes: > On 9/23/20 6:36 PM, Cleber Rosa wrote: >> On Tue, Sep 22, 2020 at 05:00:45PM -0400, John Snow wrote: >>> Annotations do not change runtime behavior. >>> This commit *only* adds annotations. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> [...] >>> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py >>> index e97b9a8e15..1cc6a5b82d 100644 >>> --- a/scripts/qapi/source.py >>> +++ b/scripts/qapi/source.py >>> @@ -11,37 +11,42 @@ >>> import copy >>> import sys >>> +from typing import List, Optional, TypeVar >>> >>> class QAPISchemaPragma: >>> - def __init__(self): >>> + def __init__(self) -> None: >> I don't follow the reason for typing this... >> >>> # Are documentation comments required? >>> self.doc_required = False >>> # Whitelist of commands allowed to return a non-dictionary >>> - self.returns_whitelist = [] >>> + self.returns_whitelist: List[str] = [] >>> # Whitelist of entities allowed to violate case conventions >>> - self.name_case_whitelist = [] >>> + self.name_case_whitelist: List[str] = [] >>> >>> class QAPISourceInfo: >>> - def __init__(self, fname, line, parent): >>> + T = TypeVar('T', bound='QAPISourceInfo') >>> + >>> + def __init__(self: T, fname: str, line: int, parent: Optional[T]): >> And not this... to tune my review approach, should I assume that >> this >> series intends to add complete type hints or not? >> > > This is a mypy quirk you've discovered that I've simply forgotten about. > > When __init__ has *no* arguments, you need to annotate its return to > explain to mypy that you have fully typed that method. It's a sentinel > that says "Please type check this class!" > > When __init__ has some arguments, you only need to type those > arguments and not the return type. The sentinel is not needed. > > __init__ *never* returns anything, so I opted to omit this useless > annotation whenever it was possible to do so. Worth capturing in a comment and/or commit message?
On 9/25/20 8:22 AM, Markus Armbruster wrote:
> Worth capturing in a comment and/or commit message?
Doesn't hurt me any to do so.
It's also good fodder for a style guide document, which would centralize
such things.
Here is a formal IOU: https://gitlab.com/jsnow/qemu/-/issues/7
--js
On Wed, Sep 23, 2020 at 07:55:50PM -0400, John Snow wrote: > On 9/23/20 6:36 PM, Cleber Rosa wrote: > > On Tue, Sep 22, 2020 at 05:00:45PM -0400, John Snow wrote: > > > Annotations do not change runtime behavior. > > > This commit *only* adds annotations. > > > > > > Signed-off-by: John Snow <jsnow@redhat.com> > > > --- > > > scripts/qapi/mypy.ini | 5 ----- > > > scripts/qapi/source.py | 31 ++++++++++++++++++------------- > > > 2 files changed, 18 insertions(+), 18 deletions(-) > > > > > > diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini > > > index 9da1dccef4..43c8bd1973 100644 > > > --- a/scripts/qapi/mypy.ini > > > +++ b/scripts/qapi/mypy.ini > > > @@ -39,11 +39,6 @@ disallow_untyped_defs = False > > > disallow_incomplete_defs = False > > > check_untyped_defs = False > > > -[mypy-qapi.source] > > > -disallow_untyped_defs = False > > > -disallow_incomplete_defs = False > > > -check_untyped_defs = False > > > - > > > > This is what I meant in my comment in the previous patch. It looks > > like a mix of commit grannurality styles. Not a blocker though. > > > > Yep. Just how the chips fell. Some files were just very quick to cleanup and > I didn't have to refactor them much when I split things out, so the > enablements got rolled in. > > I will, once reviews are in (and there is a commitment to merge), try to > squash things where it seems appropriate. > > > > [mypy-qapi.types] > > > disallow_untyped_defs = False > > > disallow_incomplete_defs = False > > > diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py > > > index e97b9a8e15..1cc6a5b82d 100644 > > > --- a/scripts/qapi/source.py > > > +++ b/scripts/qapi/source.py > > > @@ -11,37 +11,42 @@ > > > import copy > > > import sys > > > +from typing import List, Optional, TypeVar > > > class QAPISchemaPragma: > > > - def __init__(self): > > > + def __init__(self) -> None: > > > > I don't follow the reason for typing this... > > > > > # Are documentation comments required? > > > self.doc_required = False > > > # Whitelist of commands allowed to return a non-dictionary > > > - self.returns_whitelist = [] > > > + self.returns_whitelist: List[str] = [] > > > # Whitelist of entities allowed to violate case conventions > > > - self.name_case_whitelist = [] > > > + self.name_case_whitelist: List[str] = [] > > > class QAPISourceInfo: > > > - def __init__(self, fname, line, parent): > > > + T = TypeVar('T', bound='QAPISourceInfo') > > > + > > > + def __init__(self: T, fname: str, line: int, parent: Optional[T]): > > > > And not this... to tune my review approach, should I assume that this > > series intends to add complete type hints or not? > > > > This is a mypy quirk you've discovered that I've simply forgotten about. > > When __init__ has *no* arguments, you need to annotate its return to explain > to mypy that you have fully typed that method. It's a sentinel that says > "Please type check this class!" > Ouch. Is this a permanent quirk or a known bug that will eventually be addressed? - Cleber.
On 9/25/20 1:05 PM, Cleber Rosa wrote: > On Wed, Sep 23, 2020 at 07:55:50PM -0400, John Snow wrote: >> On 9/23/20 6:36 PM, Cleber Rosa wrote: >>> On Tue, Sep 22, 2020 at 05:00:45PM -0400, John Snow wrote: >>>> Annotations do not change runtime behavior. >>>> This commit *only* adds annotations. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> scripts/qapi/mypy.ini | 5 ----- >>>> scripts/qapi/source.py | 31 ++++++++++++++++++------------- >>>> 2 files changed, 18 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini >>>> index 9da1dccef4..43c8bd1973 100644 >>>> --- a/scripts/qapi/mypy.ini >>>> +++ b/scripts/qapi/mypy.ini >>>> @@ -39,11 +39,6 @@ disallow_untyped_defs = False >>>> disallow_incomplete_defs = False >>>> check_untyped_defs = False >>>> -[mypy-qapi.source] >>>> -disallow_untyped_defs = False >>>> -disallow_incomplete_defs = False >>>> -check_untyped_defs = False >>>> - >>> >>> This is what I meant in my comment in the previous patch. It looks >>> like a mix of commit grannurality styles. Not a blocker though. >>> >> >> Yep. Just how the chips fell. Some files were just very quick to cleanup and >> I didn't have to refactor them much when I split things out, so the >> enablements got rolled in. >> >> I will, once reviews are in (and there is a commitment to merge), try to >> squash things where it seems appropriate. >> >>>> [mypy-qapi.types] >>>> disallow_untyped_defs = False >>>> disallow_incomplete_defs = False >>>> diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py >>>> index e97b9a8e15..1cc6a5b82d 100644 >>>> --- a/scripts/qapi/source.py >>>> +++ b/scripts/qapi/source.py >>>> @@ -11,37 +11,42 @@ >>>> import copy >>>> import sys >>>> +from typing import List, Optional, TypeVar >>>> class QAPISchemaPragma: >>>> - def __init__(self): >>>> + def __init__(self) -> None: >>> >>> I don't follow the reason for typing this... >>> >>>> # Are documentation comments required? >>>> self.doc_required = False >>>> # Whitelist of commands allowed to return a non-dictionary >>>> - self.returns_whitelist = [] >>>> + self.returns_whitelist: List[str] = [] >>>> # Whitelist of entities allowed to violate case conventions >>>> - self.name_case_whitelist = [] >>>> + self.name_case_whitelist: List[str] = [] >>>> class QAPISourceInfo: >>>> - def __init__(self, fname, line, parent): >>>> + T = TypeVar('T', bound='QAPISourceInfo') >>>> + >>>> + def __init__(self: T, fname: str, line: int, parent: Optional[T]): >>> >>> And not this... to tune my review approach, should I assume that this >>> series intends to add complete type hints or not? >>> >> >> This is a mypy quirk you've discovered that I've simply forgotten about. >> >> When __init__ has *no* arguments, you need to annotate its return to explain >> to mypy that you have fully typed that method. It's a sentinel that says >> "Please type check this class!" >> > > Ouch. Is this a permanent quirk or a known bug that will eventually > be addressed? Permanent, it is a feature. mypy intentionally supports gradual typing as a paradigm: it allows you to intermix "typed" and "untyped" functions. ``` def __init__(self): pass ``` Happens to pass as both untyped and fully typed. In order to distinguish it in this one case, you must add the return annotation as a declaration of intent. However, when using '--strict' mode, you are declaring your intent to mypy that everything MUST be strictly typed, so perhaps in this case it would be possible to omit the annotation for __init__. So maybe someday this will change; but given how uncommon it is to write no-argument init methods, I am hardly bothered by it. Mypy will remind you if you forget. > > - Cleber. >
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini index 9da1dccef4..43c8bd1973 100644 --- a/scripts/qapi/mypy.ini +++ b/scripts/qapi/mypy.ini @@ -39,11 +39,6 @@ disallow_untyped_defs = False disallow_incomplete_defs = False check_untyped_defs = False -[mypy-qapi.source] -disallow_untyped_defs = False -disallow_incomplete_defs = False -check_untyped_defs = False - [mypy-qapi.types] disallow_untyped_defs = False disallow_incomplete_defs = False diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py index e97b9a8e15..1cc6a5b82d 100644 --- a/scripts/qapi/source.py +++ b/scripts/qapi/source.py @@ -11,37 +11,42 @@ import copy import sys +from typing import List, Optional, TypeVar class QAPISchemaPragma: - def __init__(self): + def __init__(self) -> None: # Are documentation comments required? self.doc_required = False # Whitelist of commands allowed to return a non-dictionary - self.returns_whitelist = [] + self.returns_whitelist: List[str] = [] # Whitelist of entities allowed to violate case conventions - self.name_case_whitelist = [] + self.name_case_whitelist: List[str] = [] class QAPISourceInfo: - def __init__(self, fname, line, parent): + T = TypeVar('T', bound='QAPISourceInfo') + + def __init__(self: T, fname: str, line: int, parent: Optional[T]): self.fname = fname self.line = line self.parent = parent - self.pragma = parent.pragma if parent else QAPISchemaPragma() - self.defn_meta = None - self.defn_name = None + self.pragma: QAPISchemaPragma = ( + parent.pragma if parent else QAPISchemaPragma() + ) + self.defn_meta: Optional[str] = None + self.defn_name: Optional[str] = None - def set_defn(self, meta, name): + def set_defn(self, meta: str, name: str) -> None: self.defn_meta = meta self.defn_name = name - def next_line(self): + def next_line(self: T) -> T: info = copy.copy(self) info.line += 1 return info - def loc(self): + def loc(self) -> str: if self.fname is None: return sys.argv[0] ret = self.fname @@ -49,13 +54,13 @@ def loc(self): ret += ':%d' % self.line return ret - def in_defn(self): + def in_defn(self) -> str: if self.defn_name: return "%s: In %s '%s':\n" % (self.fname, self.defn_meta, self.defn_name) return '' - def include_path(self): + def include_path(self) -> str: ret = '' parent = self.parent while parent: @@ -63,5 +68,5 @@ def include_path(self): parent = parent.parent return ret - def __str__(self): + def __str__(self) -> str: return self.include_path() + self.in_defn() + self.loc()
Annotations do not change runtime behavior. This commit *only* adds annotations. Signed-off-by: John Snow <jsnow@redhat.com> --- scripts/qapi/mypy.ini | 5 ----- scripts/qapi/source.py | 31 ++++++++++++++++++------------- 2 files changed, 18 insertions(+), 18 deletions(-)