Fill in gaps in type annotations. #235
Conversation
SethMMorton
force-pushed
the
main
branch
2 times, most recently
from
2dc8165 to
b0d5e83
Compare
|
@jaraco Let me know if I have to modify any of my changes to conform to a style guide. |
|
Thanks @SethMMorton for investigating and working on a solution.
My first instinct is that this change takes the project in the wrong direction. Ideally, there should be no need for a stub file and it should be possible to put annotations directly on the code (e.g. #221). The fact that the ecosystem doesn't allow for an incremental transition from a stub file to inline annotations is quite annoying. That seems like a deficiency in mypy and not in this project. In any case, we need to work toward moving all of the annotations out of the stub file and into the source file, and not the reverse.
This change is problemmatic. This project builds on jaraco/skeleton in order to manage systematic concerns across projects in a shared manner (versus applying bespoke maintenance on each project manually). That approach is intended to rely on pytest-mypy to perform type checks. If mypy checks aren't being invoked, that's a bug in the skeleton or elsewhere upstream. I do see what you're saying - running tests in verbose mode, I don't see mypy running. Looking at pyproject.toml, I see that mypy is disabled due to jaraco/skeleton#143. I know a lot of progress has been made in getting mypy working again in a number of skeleton-based projects, so it may be time to revert that disablement. Perhaps I'll work on that today.
Again, this concern is managed at jaraco/skeleton and should avoid diverging from that technique unless there's a very strong reason to do so. Would you be interested instead in working on a PR to migrate the type annotations to source code? |
|
Note that with #233, mypy checks are once again enabled for this project. |
|
Sure - I prefer annotations in-source as well. I'll update this PR to remove the stub files and move all annotations in-source. Just to make sure I understand, I should remove all changes to how |
Remove classes.pyi.
Remove matchers.pyi.
__init__.pyi has been removed. Annotations were copied from the stuf file as-is, without changing either the annotations or anything about the source code (with exceptions mentioned below). In some cases, the annotations are not correct (or better ones could be made) or mypy will now flag code inside the annotated methods - these will be corrected in subsequent commits. This commit is intended to serve as a reference point for translation from stub annotations to in-source annotations. The exceptions to as-is copy are the following: - When the annotation for an input parameter was "Path | str" it has been replaced with "str" since "Path" is a subclass of "str" - When the return type was "Path" it has been replaced with "Self" (except in the cases where it truely should be "Path" like in "TempDir") - In cases where the stub contained explicit parameters and the source had "*args, **kwargs", these have been replaced with explicit parameters to match the stub. In several of these cases, this required changing the source code to call the forwarded function with the explict parameters.
The ns keyword argument can be either present or not, but it has no default. To specify this requires overloads and **kwargs.
The base Path does not have a _validate, but subclasses do. The base Path.__init__ calls _validate, but wraps it in an AttributeError suppression. mypy still throws an error despite. To solve, a do-nothing _validate function has been added to Path itself, and the AttributeError suppression has been removed.
The Number ABC is more abstract than needed when defining allowed input types - float captures both float and int. Further, when returning explictly an int, the return type should be annotated as such instead of as Number. This also eliminates a mypy error about multiplying a Number with a float.
mypy has a hard time if the same name is used to hold more than one type, so where this error was occuring a new name is used for the second type.
The following changes had to be made: - In Path.walk, Handlers._resolve transformed "errors" from a string into a function, which mypy does not like. To resolve, it now assignes "error_fn". - In the recursive Path.walk call, error_fn is not of type str for the errors argument, which would be a typing violation - this has been explictly ignored so as to not make the function option public. - mypy likes to use isinstance to differentiate types, so the logic for Handers._resolve was made more mypy-friendly so that it properly understood the types it was dealing with. - To avoid expected argument errors, the "public" Handlers functions have been made into staticmethods.
When calling e.g. "self.open()", mypy did not recognize that that the implict self argument was acting as the first argument to the builtin open function. Unfortunately, to make mypy fully understand the Path.open function a full list of the open overloads is required.
A tuple of exceptions was passed to contextlib.suppress, but it actually needs to be individual arguments of exceptions - this has been corrected.
mypy has a hard knowing what is being returned with the mixin design pattern, so instead of attempting some sort of refactoring these errors are simply being ignored.
mypy cannot use aribrary logic to conditionally run code during type checking - it mostly only uses isinstance, and checks against sys.version_info or sys.platform. The conditional inclusion of methods into Path have been changed from using hasattr or a globals check to sys.platform. Additionally, the conditional addition of shutil.move has been removed since shutil.move is always available.
When used in the functional form the typing system has a hard time determining the types of properties. Changing the definition to use the decorator form resolves this issue.
SethMMorton
force-pushed
the
main
branch
3 times, most recently
from
a0fbbc0 to
86d020f
Compare
When assigning a shutil function directly into the Path namespace, the typing system seems to have trouble figuring out that the first argument is the implict self, and as a result the type checking fails for these functions. To solve, thin passthrough functions have been added so that the annotations are correct for the Path context, but the docstrings have been copied from shutil as-is.
Added annotations to functions that did not strictly need them but adding them improves editor highlighing.
Type hinting was taking its cue from the multimethod decorator, so joinpath was always returning Any as the type. To resolve, the multimethod decorator has been annotated with generics so that the typing system can correctly infer the return type of joinpath.
The union for _Match is not defined as part of a function annotation, so the "annotations" import from __future__ does not take effect and thus results in a syntax error on Python < 3.10. To fix, the Union annotation is used for this one type definition.
The implementations for get_owner need to be hiddent behind sys.platform in order for mypy to not complain about non-existing functionality on some platforms.
|
@jaraco OK, here's the re-attempt at this PR. It's pretty different, since the changes are in-source. The first several commits are directly translating the stub annotations into the in-source annotations. After that are many commits that resolve the mypy errors that result. The attempt was to be atomic with the commits, but if you want me to squash that is OK. I see that coverage is failing now, likely because of all the platform-dependent branches that were added. I can add no-cover statements if desired. I also see the docs test is failing... I'll need some guidance there. |
Yes, please. |
Don't worry about coverage if you haven't changed the coverage profile. |
The docs errors are caused by |
I'm happy to retain the history of a change (and let the merge commit represent the overall effect). |
|
OK - |
|
I just realized this PR likely resolves #215, correct? |
This is intended to address issue #234.
It turns out that
mypydoes not look at the annotations in the main.pyfile if a stub.pyifile exists. So, functionality added since the.pyifiles were created that had annotations in-place were not having those annotations exported, resulting in a small subset of functionality missing type hints for this module.These gaps have been addressed.
The following related changes have also been made:
mypy.stubtestto verify that the.pyiand.pyfiles are in sync, ensuring this does not happen in the futuremypyinstead of using thepytest-mypyplugin (this plugin was not actually runningmypy)path.Path.joinpath()has been fixed so that it no longer reports its return type asAnyCloses #215