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

Address type issues #1751

Merged
merged 20 commits into from
May 18, 2023
Merged

Conversation

cspotcode
Copy link
Collaborator

@cspotcode cspotcode commented May 5, 2023

Mypy is unable to typecheck against pyglet and other deps that don't include a py.typed file. So it's unable to catch certain issues within arcade's own codebase. Pyright has an option telling it to use the source code of pyglet as types. I couldn't find an equivalent option for mypy.

Yeah, some of the diagnostics are mere nuisances, unlikely to cause real problems. But some are legit.

This PR tweaks the pyright configuration and also makes minor changes to code to address some of the diagnostics. I'm not sure the best way to solicit feedback, ideally the GUI changes would be reviewed by someone familiar with the GUI code, etc.

I haven't fixed all the issues, but I want to pause this work till I get the team's temperature re: making these changes & enabling pyright on CI. I don't want to impose unwelcome work on the team.

@cspotcode
Copy link
Collaborator Author

Note to self: merge with #1720, check for conflicts and type errors.

@gran4
Copy link
Contributor

gran4 commented May 6, 2023

How would that work?

@cspotcode
Copy link
Collaborator Author

I see that you are making some of the same improvements to types that I am; changing int to float. In my case, I'm doing it because pyright is catching the mistakes. I've improved the pyright configuration allowing it to catch stuff that it could not before:

https://github.com/pythonarcade/arcade/pull/1751/files#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711

So I'm thinking we can somehow test your type changes against my improved pyright configuration, to see if they're compatible.

Is that what you were asking?

Copy link
Member

@eruvanos eruvanos left a comment

Choose a reason for hiding this comment

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

I read through all files but in most gl parts I am unsure.
The changes for the GUI look good, especially if they work ;)

The | operator should not be used due to the Python 3.10 requirement.

arcade/camera.py Outdated Show resolved Hide resolved
arcade/gl/buffer.py Outdated Show resolved Hide resolved
arcade/gl/compute_shader.py Outdated Show resolved Hide resolved
arcade/gui/examples/scroll_area.py Outdated Show resolved Hide resolved
arcade/gui/property.py Outdated Show resolved Hide resolved
@eruvanos
Copy link
Member

eruvanos commented May 9, 2023

@pvcraven maybe it is time to move to pyright.
I tested it also during my work time and it works better (for us) then mypy.
It looks also like it has a better detection and found actual bugs, which mypy did not detect.

@pvcraven
Copy link
Member

pvcraven commented May 9, 2023

That's fine by me. I'd started a while back fixing some things, glad to see some other people working on it.

@pvcraven
Copy link
Member

pvcraven commented May 9, 2023

Yes, we don't want to use the | operator yet. It is awesome, I agree, but we still need python 3.9 compatibility.

@cspotcode cspotcode marked this pull request as ready for review May 10, 2023 21:12
@cspotcode
Copy link
Collaborator Author

This PR is ready to merge.

I've moved all the arcade.gl changes to another PR #1767 for @einarf(?) to review, to avoid blocking this PR from merging.

I've also addressed review feedback. Linting and tests are passing locally, hopefully will be passing on CI too.

@pvcraven pvcraven requested a review from eruvanos May 10, 2023 21:43
@pvcraven
Copy link
Member

Cool, we'll see if @eruvanos has a sec to double check, but I think his concerns were addressed.

Copy link
Member

@eruvanos eruvanos left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, from my point of view, all remaining changes (beside the asserts) look good.
Will merge as soon as they are replaced.

Again thank you for all your work!

arcade/tilemap/tilemap.py Outdated Show resolved Hide resolved
arcade/tilemap/tilemap.py Outdated Show resolved Hide resolved
@cspotcode
Copy link
Collaborator Author

I created pythonarcade/pytiled_parser#69 to track changes to type annotations in pytiled_parser which eliminate the need for assert in arcade. Per Discord conversation, we can merge this PR now, which unblocks others. And we can remove the if TYPE_CHECKING: asserts in the future.

Copy link
Member

@eruvanos eruvanos left a comment

Choose a reason for hiding this comment

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

Great!

@eruvanos eruvanos merged commit 8e1348a into pythonarcade:development May 18, 2023
@eruvanos eruvanos mentioned this pull request May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants