WIP: Typed Agument Clinic #44
Conversation
erlend-aasland
force-pushed
the
clinic-typed
branch
from
3128ef1
to
d2ec2ef
Compare
|
cc. @AlexWaygood |
|
So, I basically just ran |
| @@ -1915,8 +1916,11 @@ def dump(self): | |||
| # maps strings to Language objects. | |||
| # "languages" maps the name of the language ("C", "Python"). | |||
| # "extensions" maps the file extension ("c", "py"). | |||
| L = TypeVar('L', bound=Language) | |||
| LangDict = dict[str, L] | |||
There was a problem hiding this comment.
I'm slightly surprised I cannot just do this:
LangDict = dict[str, Language]
There was a problem hiding this comment.
What's the error you get if you do that? I was going to suggest you do that!
There was a problem hiding this comment.
Tools/clinic/clinic.py:1922: error: Value expression in dictionary comprehension has incompatible type "Type[CLanguage]"; expected type "Language" [misc]
Tools/clinic/clinic.py:1923: error: Incompatible types in assignment (expression has type "Type[PythonLanguage]", target has type "Language") [assignment]
Found 2 errors in 1 file (checked 1 source file)
There was a problem hiding this comment.
Right, so it should be LangDict = dict[str, type[Language]] rather than dict[str, Language] or dict[str, L]. The values are class objects, rather than instances of the class :)
There was a problem hiding this comment.
For some heated debate on whether the type-abstract error code is useful at all, you can see python/mypy#4717
There was a problem hiding this comment.
Seems like this is a bug in mypy (Language is a class with an abc metaclass)
There was a problem hiding this comment.
I'll just revert to what I had earlier
There was a problem hiding this comment.
Hmm, what you had earlier is incorrect :/
Mypy is working as intended here -- it's a question of the design choice mypy has made here rather than a bug, per se. Mypy believes that if you have a type[Foo] annotation, you'll probably want to instantiate Foo at some point, so it's therefore unsound to have a type[Foo] annotation if Foo is an abstract class, since abstract classes can't be instantiated. In this case Language is an abstract class (it has an abc metaclass and abstract methods, so it can't be instantiated). So mypy is working as designed.
However, arguably this lint is based on some flawed premises and makes life unduly complicated in lots of situations. (See python/mypy#4717 for detailed debate on this point.)
You can work around the issue by using the Callable[[str], Language] annotation I suggested above (which is just as valid as type[Language] here), or disable the error code. But using a TypeVar is incorrect here, since there's no type-dependent relationship going on.
There was a problem hiding this comment.
Got it; I'll investigate tomorrow. (FYI, your comments did not show up until now.)
|
By the way, some stricter mypy command-line options that I recommend enabling right from the beginning are: (These can all be enabled via a config file as well as via command-line arguments. I usually use a |
- ditch no-return - use callable from collections.abc - use namedtuple from typing
|
Great, thanks for your help and patience so far :) |
|
CC. @hauntsaninja |
|
FYI, I currently have python/pyperformance#286 open to add a basic mypy check to pyperformance — could be useful as a template for what the GitHub Actions workflow and config file might look like! |
Oh, that's neat! Thanks for the heads-up. We should start with something similar over at the CPython repo. |
No description provided.