Define types for AsyncIOWrapper and trio.Path#2706
Define types for AsyncIOWrapper and trio.Path#2706jakkdl merged 31 commits intopython-trio:masterfrom
AsyncIOWrapper and trio.Path#2706Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2706 +/- ##
=======================================
Coverage 98.89% 98.89%
=======================================
Files 113 113
Lines 16490 16552 +62
Branches 3000 3006 +6
=======================================
+ Hits 16307 16369 +62
Misses 126 126
Partials 57 57
|
TeamSpen210
force-pushed
the
typed-filewrapper
branch
from
1d7b637 to
2bb8bf1
Compare
jakkdl
left a comment
jakkdl
left a comment
There was a problem hiding this comment.
Looks ... about as good as it could look 😅
I would like to see you enable stricter checks for these files in pyproject.toml as I've done in my PR's: https://github.com/python-trio/trio/pull/2705/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711
At least disallow_untyped_defs - but disallow_any_expr would be nice as well. My thinking was that we'll gradually allow those for all files (at some point, transitioning to it being an ignore-list instead of an enable-list).
Are there really no changes to _tests/verify_types.json? Looks like there's 14 lines referencing _path atm:
trio/trio/_tests/verify_types.json
Lines 106 to 119 in 6f187fb
and open_file & wrap_file seems to be in there as well.
| FileT = TypeVar("FileT") | ||
| FileT_co = TypeVar("FileT_co", covariant=True) | ||
| T = TypeVar("T") | ||
| T_co = TypeVar("T_co", covariant=True) | ||
| T_contra = TypeVar("T_contra", contravariant=True) | ||
| AnyStr_co = TypeVar("AnyStr_co", str, bytes, covariant=True) | ||
| AnyStr_contra = TypeVar("AnyStr_contra", str, bytes, contravariant=True) |
There was a problem hiding this comment.
These types might also warrant some comments. One day I'll get comfortable with co- & contravariant - but some brief comments here saying which ones are used where and why will probably help readers and any future people trying to modify it.
There was a problem hiding this comment.
I mainly picked/needed them because protocols complain if you don't have the right ones :)
These are way too dynamic to properly type.
Conflicts: - trio/_path.py
A5rocks
left a comment
A5rocks
left a comment
There was a problem hiding this comment.
My eyes glazed over when looking over trio/_path.py. I'll take a look later.
trio/_file_io.py
Outdated
|
|
||
| class _HasNewlines(Protocol): | ||
| @property | ||
| def newlines(self) -> Any: ... # None, str or tuple |
There was a problem hiding this comment.
I don't really understand this comment.
There was a problem hiding this comment.
I preserved it from the io type hints, looks like the docs say this returns those but typeshed chose to make it Any. Actually we could just use a typevar here to do whatever the file object says it returns.
| newline: str | None = None, | ||
| closefd: bool = True, | ||
| opener: _Opener | None = None, | ||
| ) -> AsyncIOWrapper[IO[Any]]: |
There was a problem hiding this comment.
I don't really like this Any but I suppose practicality over purity...
There was a problem hiding this comment.
It corresponds to what's in typeshed for regular open. That overload's only going to be taken for a non-literal mode, where we don't really have any idea what IO object we're getting...
|
Posting in this issue too that type hinting |
A5rocks
left a comment
A5rocks
left a comment
There was a problem hiding this comment.
I'm just going to trust this is all good!
|
🚀 |
4 participants
This fully types both these wrapper classes. Dealing with the complicated hierarchy of IO methods was tricky, until I realised self-type-protocols can be used to make
AsyncIOWrapperonly have the methods that the underlying file does. It does unfortunately require a rather verbose set of definitions. It could be simplified a little by grouping methods that always are defined together, but that's less accurate.Due to the length of the stub definitions, I did turn off Black formatting there so they could be formatted compactly like type stubs, otherwise the line count would get much longer and harder to read. Not sure what's desirable either way?