Skip to content

Conversation

lev-blit
Copy link
Contributor

@lev-blit lev-blit commented Oct 1, 2025

Didn't do all of them but this is hopefully a nice start :)

This comment has been minimized.

@lev-blit lev-blit marked this pull request as ready for review October 1, 2025 16:21
Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks mostly good. A few remarks.

Comment on lines 13 to 14
# TODO: handle requires the type pywin32._win32typing.PyHANDLE
def __init__(self, handle=None) -> None: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another workaround is to add a type alias like this above and use that here:

_PyHANDLE: TypeAlias = Any  # TODO: pywin32._win32typing.PyHANDLE

Comment on lines 21 to 22
# TODO: return type requires the type pywin32._win32typing.PyHANDLE
def detach(self) -> Incomplete | None: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

def check_resource(resource_name: str): ...
def minimum_version(version: str): ...
def update_headers(f: Callable[..., Incomplete]): ...
def update_headers(f: Callable[..., Any]): ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering that this is a decorator, we should either use type var and use the same type var for the returned callable, or just leave it as Incomplete for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used paramspec + typevar in the end :)

def parse_devices(devices: Iterable[str | dict[str, Incomplete]]) -> list[dict[str, Incomplete]]: ...
def kwargs_from_env(environment: Mapping[str, Incomplete] | None = None) -> _EnvKWArgs: ...
def parse_devices(devices: Iterable[str | dict[str, str]]) -> list[dict[str, str]]: ...
def kwargs_from_env(environment: Mapping[str, AnyStr] | None = None) -> _EnvKWArgs: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AnyStr is a type var and as such needs a second use. You probably just want to use str | bytes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did some more searching around and it seems it is only used as str and not bytes | str :)

def GetOverlappedResult(hFile: int, overlapped: _win32typing.PyOVERLAPPED, bWait: int | bool, /) -> int: ...
def WaitNamedPipe(pipeName: str, timeout, /) -> None: ...
def GetNamedPipeInfo(hNamedPipe: int, /) -> tuple[Incomplete, Incomplete, Incomplete, Incomplete]: ...
def GetNamedPipeInfo(hNamedPipe: int, /) -> tuple[int, int, int, int]: ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this change to a separate PR?

@lev-blit
Copy link
Contributor Author

lev-blit commented Oct 7, 2025

Thank you for the detailed review!

Copy link
Contributor

github-actions bot commented Oct 7, 2025

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants