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

Properties with setters lack type annotations #272

Open
aatle opened this issue Feb 19, 2025 · 1 comment
Open

Properties with setters lack type annotations #272

aatle opened this issue Feb 19, 2025 · 1 comment

Comments

@aatle
Copy link

aatle commented Feb 19, 2025

Problem:
Almost all of the settable properties (such as Body.mass) are dynamically defined, so mypy doesn't recognize them. The properties get inferred to be of type Any (from the mixin), which is undesirable.

Suggestion:
Rewrite most of the writable properties in normal syntax, putting their docstring in the getter function. Static type checkers can then correctly see their type.
I can make the PR.

Exception:
Some properties are of Vec2d and so their setter accepts tuples, a different type than the getter. It has been pointed out before that type checkers don't recognize this.
So, these properties will be left alone.
Note: Mypy 1.16 adds support for getters and setters with different types. See the release notes.

@viblo
Copy link
Owner

viblo commented Feb 20, 2025

Ah, I think there was some reason why I used the property function.. but now I dont remember. And regardless, seems like it all works as it should using the decorator nowadays so I think it would be a good improvement.

If you make a PR, great! The best is if you can do it against the pymunk7 branch ( https://github.com/viblo/pymunk/tree/pymunk7 ), since Im trying to finalize 7.0 there. Less risk of merge issues that way.

I saw they fixed it in mypy, nice! 🎉🎉 I even posted in the issue for it 5 years ago 😄 I wonder how quickly they will release. It would be nice if fixing Vec2d could happen at the same time (I mean with Pymunk 7.0), but who knows, might take a long time.. Anyway, if you do a PR for the rest I can also see when everything is set for 7.0 if this last bit can be done as well.

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

No branches or pull requests

2 participants