-
Notifications
You must be signed in to change notification settings - Fork 41
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 orm.query
#70
Conversation
sqlalchemy-stubs/orm/query.pyi
Outdated
def yield_per(self, count: Any) -> None: ... | ||
def get(self, ident: Any): ... | ||
) -> _TQuery: ... | ||
def yield_per(self: _TQuery, count: Any) -> _TQuery: ... |
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.
count should an int I think
sqlalchemy-stubs/orm/query.pyi
Outdated
) -> Any: ... | ||
def merge_result(self, iterator: Any, load: bool = ...) -> Any: ... | ||
def exists(self) -> Exists: ... | ||
def count(self) -> Any: ... |
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.
should it return int?
sqlalchemy-stubs/orm/query.pyi
Outdated
@property | ||
def session(self): ... | ||
def session(self) -> Any: ... |
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.
I guess this should be session protocol
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.
Optional[_SessionProtocol]
, but yes
sqlalchemy-stubs/orm/session.pyi
Outdated
@@ -406,7 +406,9 @@ class Session(_SessionClassMethods): | |||
binds: Optional[Mapping[Any, Union[Connection, Engine]]] = ..., | |||
enable_baked_queries: bool = ..., | |||
info: Optional[Mapping[Any, Any]] = ..., | |||
query_cls: Optional[Union[Query, Callable[..., Query]]] = ..., | |||
query_cls: Optional[ | |||
Union[Query[Any], Callable[..., Query[Any]]] |
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.
shouldn't this be Type[Query[Any]]?
@@ -473,7 +475,7 @@ class Session(_SessionClassMethods): | |||
_sa_skip_events: Optional[Any] = ..., | |||
_sa_skip_for_implicit_returning: bool = ..., | |||
) -> Union[Connection, Engine]: ... | |||
def query(self, *entities: Any, **kwargs: Any) -> Query: ... | |||
def query(self, *entities: Any, **kwargs: Any) -> Query[Any]: ... |
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.
should we make an overload here similar to this:
def query(self, entities: Type[_T], **kwargs: Any) -> Query[_T]
?
not sure if others are also applicable
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.
I'm not sure that's right and it's the reason I left Query.__init__
basically untyped. It uses coerce.expect()
, so it can be a lot of different things.
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.
Without is I don't think we support the case: things = session.query(Thing).all()
and things is not List[Thing]
, no?
Description
Improve typings for
orm.query
. Fixes #66.Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>
in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>
in the commit messageHave a nice day!