Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
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
---
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.
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
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
> > > ---
> > > 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.
signature.asc
Description: PGP signature
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
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
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
John Snow 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
[...]
>>> 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?
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
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
---
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.
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
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
> ---
> 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.
signature.asc
Description: PGP signature
Re: [PATCH v2 22/38] qapi/source.py: add type hint annotations
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 Reviewed-by: Eduardo Habkost -- Eduardo
[PATCH v2 22/38] qapi/source.py: add type hint annotations
Annotations do not change runtime behavior.
This commit *only* adds annotations.
Signed-off-by: John Snow
---
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
-
[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()
--
2.26.2
