add types to io_windows #2761
add types to io_windows #2761
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #2761 +/- ##
=======================================
Coverage 98.97% 98.97%
=======================================
Files 115 115
Lines 17102 17117 +15
Branches 3079 3079
=======================================
+ Hits 16927 16942 +15
Misses 121 121
Partials 54 54
|
|
I can maybe try to work on this (as I mainly use Windows) -- I would use GitHub's assignment thing that I remember it having but I can't find the buttons on mobile :( |
|
Pushed an attempt to type the CFFI parts, not sure if we want to go that route (or just leave them dynamic)? With a bunch of |
|
I don't understand the pypy failure but that does seem to be non-transient |
|
Looks like pypy on Windows is broken on master too, but since 0c108f9 so I think it's an upstream issue. |
|
|
||
|
|
||
| class WindowsIOManager: | ||
| def __init__(self): | ||
| def __init__(self) -> None: | ||
| # If this method raises an exception, then __del__ could run on a | ||
| # half-initialized object. So we initialize everything that __del__ | ||
| # touches to safe values up front, before we do anything that can | ||
| # fail. | ||
| self._iocp = None |
There was a problem hiding this comment.
Since iocp can only be None if __init__ fails (so in __del__), it might be better to just cast/type-ignore the none assignment, so we avoid asserts everywhere.
There was a problem hiding this comment.
Could also add a self._is_initialized variable that's initially set to False, and set to True after self._iocp is initialized.
There was a problem hiding this comment.
Since
iocpcan only beNoneif__init__fails (so in__del__), it might be better to just cast/type-ignore the none assignment, so we avoid asserts everywhere.
FWIW it can also be None after .close but IDK if the object still exists then.
|
Ah, thought there was something wrong with #2750 but it's rather that this PR is pushing unformatted code - i.e. need to rerun |
|
I can't approve a PR I created, but LGTM ✔️ |
| Pointer: object | ||
|
|
||
|
|
||
| class _Overlapped(Protocol): |
There was a problem hiding this comment.
Might want to just make this public, if it's being imported elsewhere.
There was a problem hiding this comment.
Yeah I'm not sure how to think about Protocol publicness as they are meant to be purely structural and any use of them could be replaced with a copy-pasted copy
| async def wait_overlapped(self, handle, lpOverlapped): | ||
| handle = _handle(handle) | ||
| async def wait_overlapped( | ||
| self, handle_: int | CData, lpOverlapped: CData | int |
There was a problem hiding this comment.
Instead of casting to/from CData, maybe we should type it as our Overlapped / Handle types, so this can be statically checked? Or just add those to the union alongside CData, to be more loose.
There was a problem hiding this comment.
I would push out lpOverlapped casting to a boundary... but looking at the code, this is public :(
The type casting can probably be improved though.
| from typing import TYPE_CHECKING, ContextManager | ||
|
|
||
| if TYPE_CHECKING: | ||
| from .._file_io import _HasFileNo |
There was a problem hiding this comment.
Seems iffy to be importing a private name from a different module, might want to make this also public.
|
@TeamSpen210 just wanted to make sure you saw my responses and think it's still fine! (mainly about |
|
That does make sense, it’s of course easier to make something public than the reverse. |
I hit a wall implementing this and I found it a hassle not being on windows, so would be lovely if anybody would take it over