-
-
Notifications
You must be signed in to change notification settings - Fork 453
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
Improve GeoDjango type-hints #1299
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow! Thanks a lot! This is amazing!
But, I literally have 0 experience with gis
.
Maybe @intgr or @adamchainz have more knowledge?
Nope, I have no experience with Django GIS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on a cursory read. As noted above, the TypedDjango team doesn't use GeoDjango. I'm inclined to just merge this as is, it's filling out types that used to be just Any
.
Changes like this are rarely perfect and we shouldn't expect them to be. If it causes regressions for users, they will be reported and can be improved.
There was some overlap between this PR and #1265 (which just got merged to master). I have pushed a merge with the latest master. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking into this some more, there are lots of re-exports that look like they shouldn't be re-exports.
Usually Django only has re-exports in __init__.py
files. Also items in explicit module __all__ = [...]
should be re-exported. The remaining typically should not be.
PS: I don't expect you to fix all the pre-existing incorrect re-exports. Just fix the new ones you have added in this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Waiting for the CI to finish.
Maybe @mjakob can try this out and share some feedback. |
I have made things!
contrib/gis/**/__init__.py
Related issues
No