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

Support Type Arguments on Query #10

Open
cancan101 opened this issue Mar 24, 2021 · 11 comments
Open

Support Type Arguments on Query #10

cancan101 opened this issue Mar 24, 2021 · 11 comments
Labels
awaiting pep Required behavior is not yet implemented and there's a pep for it enhancement New feature or request

Comments

@cancan101
Copy link

Is your feature request related to a problem? Please describe.
Currently https://github.com/dropbox/sqlalchemy-stubs allows specifying a type on a query eg Query[MyModel] whereas doing that in this library yields: error: "Query" expects no type arguments, but 1 given

Describe the solution you'd like
Allow type arguments on Query

@cancan101 cancan101 added the requires triage New issue that requires categorization label Mar 24, 2021
@zzzeek zzzeek added question Further information is requested and removed requires triage New issue that requires categorization labels Mar 24, 2021
@zzzeek
Copy link
Member

zzzeek commented Mar 24, 2021

hey there -

this is not reasonable for the reason that Mypy and Python 3.9 don't yet support variadic generics so this cannot be implemented. The case of Query(ASingleClass) is a degenerate case, any number of arguments are accepted.

@cancan101
Copy link
Author

How is this issue here different from how the other project handles it: https://github.com/dropbox/sqlalchemy-stubs/blob/55470ceab8149db983411d5c094c9fe16343c58b/sqlalchemy-stubs/orm/query.pyi#L13

@zzzeek
Copy link
Member

zzzeek commented Mar 25, 2021

I think sqlalchemy-stubs is doing it incorrectly. query does not accept a single type it accepts any number of types. Eg.:


     q1: Query[User] = session.query(User)   

     q2: Query[???] = session.query(User.id, User.name)

     q3: Query[???] = session.query(User, func.count(Address.id))

    etc.



@zzzeek
Copy link
Member

zzzeek commented Mar 25, 2021

Query is also legacy API. the issue is even more difficult to implement for 1.4/2.0 style queries which return Row() objects.

@cancan101
Copy link
Author

That seems solvable with a generic on the Row object, no?

@cancan101
Copy link
Author

And FWIW, being able to get typing on these is quite convenient: https://github.com/dropbox/sqlalchemy-stubs/blob/55470ceab8149db983411d5c094c9fe16343c58b/sqlalchemy-stubs/orm/query.pyi#L80-L86:

    def all(self) -> List[_T]: ...
    def first(self) -> Optional[_T]: ...
    def one_or_none(self) -> Optional[_T]: ...
    def one(self) -> _T: ...
    def __iter__(self) -> Iterator[_T]: ...

@zzzeek
Copy link
Member

zzzeek commented Mar 25, 2021

good mypy support is one of the main reasons why I went through the effort to replace Query entirely. Query is legacy stuff and I want people to migrate towards 1.4/2.0 style usage. in the new API, all of those methods above return a Row, not an object. If you want a list of scalars, you call the .scalars() method on Result. So you can't type Result at all against a particular user defined class, because the return type is always Row, and without variadic generics, mypy has no way to know what the individual elements of Row are.

I know it's possible because tuple(), which is currently the only type that acts this way, can do it:

from typing import Tuple

a: Tuple[int, str, bool] = (5, 'hi', True)

a[1] + 'foo'

if people are writing new code w/ pep 484 typing I want them on the new stuff. I don't want to give people any reason to stay on Query and implementing this only for Query and not the new API would feel very wrong to me.

@cancan101
Copy link
Author

Understood, but that does present a bit of a challenge when thinking of migrating from 1.3 to 1.4 on the way to 2.0. Currently I am on 1.3 using the existing stubs + mypy. When I attempted to upgrade to 1.4 and move to use the new stubs all of a sudden code that used to work with mypy no longer does.

@zzzeek
Copy link
Member

zzzeek commented Mar 25, 2021

There is already a non-compatible difference in how we implement Column, in terms of the TypeEngine e.g. Column[Integer] as opposed to Column[int]. Maybe that doesn't cause problems.

@zzzeek zzzeek added enhancement New feature or request and removed question Further information is requested labels Mar 25, 2021
@zzzeek
Copy link
Member

zzzeek commented Mar 25, 2021

because Query already has the behavior of returning a single object from .all() / one()/ etc. if it's against a single entity, and a tuple only if there is a column or multiple entities, means the way they have it at https://github.com/dropbox/sqlalchemy-stubs/blob/master/sqlalchemy-stubs/orm/session.pyi#L68 is the only way to do it anyway, and as they say the *entities case would need a plugin for a more accurate type. That "Any" they have there should be Row.

the whole thing would be much nicer with select() and Result because select() can be truly variadic in all cases.

@zzzeek
Copy link
Member

zzzeek commented Mar 25, 2021

cc @layday who is playing with the variadic stuff right now

@CaselIT CaselIT added the awaiting pep Required behavior is not yet implemented and there's a pep for it label Jul 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting pep Required behavior is not yet implemented and there's a pep for it enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants