-
Notifications
You must be signed in to change notification settings - Fork 42
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
refactor: slightly improve typing for cqlengine #197
base: master
Are you sure you want to change the base?
Conversation
seems like there's no way to properly type |
@@ -835,9 +838,13 @@ def _class_get_connection(cls): | |||
def _inst_get_connection(self): | |||
return self._connection or self.__connection__ | |||
|
|||
def __getitem__(self, s: slice | int) -> M | list[M]: |
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.
this is a bit more then adding types, this add new functionality.
I think it should come on it's own commit, with the reasoning why it's needed
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.
ah yeah, the reason this was added was mostly since I found proper typing for a model on ModelQuerySet
to be nearly impossible
@VincentRPS I've let CI run on this change, but I think it might fail on python2 for reasons™️ we still need to support python2, so this might need to wait a bit, for when we can drop python2 support completely |
seems like it's failing with python3.7
|
and as expected python2 doesn't support type annotations:
|
hmm, seeing that I'd have to say it'd be better waiting until py2 support is dropped |
thanks @VincentRPS, I'll mark this one as pending for now |
@VincentRPS, now that we remove python2 support, we can try this again |
@VincentRPS, I would recommend send it also to the upstream cassandra (don't know when they would except it) |
Seems like those changes are also breaking python3.7, so for now we can't merge those |
@fruch I thought I had already replied here but I guess I didn't, it should work with 3.7 now since I did a little fix, though I could be mistaken. |
I'll let CI run again |
I've rebase this one, and triggered CI again |
@VincentRPS could you send this to upstream? It would be better to do it there: less conflicts when merging + we don't really touch cqlengine code, so it's hard to do a proper review. |
This PR slightly improves typing for the CQLEngine.
I am having a bit of trouble though, because of the weird way Model and Queries are done,
its very hard to properly type
model.object.*
functions properly. Any ideas?