sqlalchemy / sqlalchemy2-stubs

PEP-484 typing stubs for SQLAlchemy 1.4
MIT License
159 stars 41 forks source link

Support Type Arguments on Query #10

Open cancan101 opened 3 years ago

cancan101 commented 3 years ago

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

zzzeek commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

cancan101 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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