Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Run stubtest on Windows for waitress #9181

Merged
merged 3 commits into from
Nov 13, 2022

Conversation

Avasam
Copy link
Collaborator

@Avasam Avasam commented Nov 12, 2022

Yet another stub that benefits from #8923 . Fixing stubtest failure on Windows and offering better coverage.

I did not add darwin to platforms because it would be redundant with linux. But I still added a stubtest_allowlist_darwin.txt for those who would like to run it locally on macos. (This is the edge case mentioned in #9173 )

@github-actions

This comment has been minimized.

write: Callable[..., bytes] = ...
def close(self) -> None: ...
def fileno(self) -> BytesIO: ...
if sys.platform == "linux" or sys.platform == "darwin":
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if sys.platform == "linux" or sys.platform == "darwin":
if sys.platform != "win32":

Consistent with the other check you added.

Copy link
Collaborator Author

@Avasam Avasam Nov 13, 2022

Choose a reason for hiding this comment

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

When the original code does if os.name == "posix" in waitress.trigger, the stub did if sys.platform == "linux" or sys.platform == "darwin" (there's another platform check like this in waitress.trigger).

I added if sys.platform != "win32" in waitress.server to be consistent with other if hasattr(socket, "AF_UNIX") checks.

But I can change them all to be either if you'd like.

Copy link
Member

Choose a reason for hiding this comment

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

I think the runtime checks are pretty much equivalent, and in stubs we have tended to assume all non-Windows OSes are Unix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, I'll make all 3 != "win32

@Avasam Avasam requested a review from JelleZijlstra November 13, 2022 08:10
@github-actions
Copy link
Contributor

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

@JelleZijlstra JelleZijlstra merged commit 82c0ea7 into python:main Nov 13, 2022
@Avasam Avasam deleted the stubtest-waitress-windows branch November 13, 2022 16:43
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