Skip to content

Conversation

@srittau
Copy link
Collaborator

@srittau srittau commented Aug 15, 2023

Closes: #10551

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

The runtime might include the symbols in __all__ on Windows even though it doesn't actually define them at runtime on Windows, which is a recipe for confusing our test suite

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@srittau
Copy link
Collaborator Author

srittau commented Aug 15, 2023

I've disabled stubtest for now. Alternatively we could remove __all__ from the stubs, but hopefully this will get fixed upstream.

@github-actions

This comment has been minimized.

# Currently we are in a pickle, because aiofiles.os.__all__ is incorrect at
# runtime when running on win32. (https://github.com/Tinche/aiofiles/pull/174)
# This means that stubtest is either correct on Windows or all other platforms.
skip = true
Copy link
Member

@AlexWaygood AlexWaygood Aug 15, 2023

Choose a reason for hiding this comment

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

Surely we don't need to skip stubtest entirely for the whole package. Can't we just create a stubtest_allowlist_win32.txt file in the @tests directory with three allowlist entries, aiofiles.os.__all__, aiofiles.os.sendfile and aiofiles.os.statvfs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't know we could have architecture-specific allowlists! Will change accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

One of @Avasam's innovations :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Embarrassingly, aiofiles already has architecture-specific allowlists ... 😳

Copy link
Member

Choose a reason for hiding this comment

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

One of @Avasam's innovations :)

Oh, it was @sobolevn's PR, so he deserves most of the credit! #8923 Though @Avasam pushed for the change

Copy link
Collaborator

@Avasam Avasam Aug 16, 2023

Choose a reason for hiding this comment

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

Oh, it was @sobolevn's PR, so he deserves most of the credit! #8923 Though @Avasam pushed for the change

Exact :) I opened an issue for discussion (#8660), fixed a couple edge cases (like #9173) and updated plenty of stubs afterwards.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

@github-actions
Copy link
Contributor

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

@AlexWaygood AlexWaygood merged commit 695d41f into python:main Aug 16, 2023
@srittau srittau deleted the aiofiles-23.2 branch August 16, 2023 11:17
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.

3 participants