Skip to content

Add / fix __all__ problems discovered by mypy and pyright #1708

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

Merged

Conversation

cspotcode
Copy link
Collaborator

@cspotcode cspotcode commented Apr 21, 2023

Will fix #1390

Building off the great work in #1681, and using a combination of mypy and pyright, I discovered a few more missing or broken __all__ declarations.

This PR's diff is misleading because it builds off of #1681. The diff will shrink after #1681 merges. These are the relevant changes:
https://github.com/cspotcode/arcade/compare/ae863d71159491b8726c5b1e1497fcf6085f3fa3..cspotcode_add__all__to_all

pyright's reportUnsupportedDunderAll tells us if __all__ refers to something that doesn't exist, for example "Texture" instead of "Texture2d"

mypy's attr-defined tells us when something is imported yet the importee does not declare it in __all__

@cspotcode cspotcode changed the title Add / fix __all__ discovered by mypy and pyright Add / fix __all__ problems discovered by mypy and pyright Apr 21, 2023
@cspotcode cspotcode force-pushed the cspotcode_add__all__to_all branch from c4fb49b to 49b1578 Compare April 21, 2023 22:56
@cspotcode cspotcode force-pushed the cspotcode_add__all__to_all branch from 49b1578 to 1fa8387 Compare April 22, 2023 04:23
Copy link
Member

@pushfoo pushfoo left a comment

Choose a reason for hiding this comment

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

This looks good. I cloned it locally and merged development into it. Everything still passes.

The __all__ in arcade.gui.widgetes.layout is a little long, but I think it's acceptable for now. We can split it into a multi-line version if more layouts get added.

@cspotcode
Copy link
Collaborator Author

The __all__ in arcade.gui.widgetes.layout is a little long,...

To be honest, I'm so accustomed to a formatter modifying that stuff automatically, I didn't even think about. I made it multiline

Copy link
Member

@pushfoo pushfoo left a comment

Choose a reason for hiding this comment

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

I apologize for the double review, but I took a closer look at the sprite module after remembering a pymunk experiment I did a while ago. It's worth double checking one of the entries in __all__.

Copy link
Member

@pushfoo pushfoo left a comment

Choose a reason for hiding this comment

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

Looks good.

@Cleptomania Cleptomania merged commit b7d149a into pythonarcade:development Apr 26, 2023
@cspotcode cspotcode deleted the cspotcode_add__all__to_all branch May 27, 2023 15:26
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.

Missing __all__ in some modules prevents mypy --strict from raising useful diagnostics
3 participants