sqlalchemy-stubs
sqlalchemy-stubs copied to clipboard
Typed queries
Goal of this PR is to type orm's Query objects. This will allow to type-check stuff like:
session.query(Employee) # -> Query[Employee]
session.query(Employee).get(123) # -> Employee
session.query(Employee, Invoice) # -> Query[Tuple[Employee, Invoice]]
session.query(Employee, Invoice).all() # -> List[Tuple[Employee, Invoice]]
Ok, so currently this PR handles model classes and columns, while gracefully downgrading to Any for other cases.
One thing I'm struggling with is keyed access to tuples returned by query. Under the hood SQLAlchemy defines sqlalchemy.util._collections.AbstractKeyedTuple which is a private class.
Then it provides sqlalchemy.util.KeyedTuple which implements AbstractKeyTuple. The catch is that query doesn't really use KeyedTuple. Instead it uses dynamically-created sub-classes of _LW (which inherits from AbstractKeyedTuple).
- It feels kind of dirty to use
sqlalchemy.util._collection.AbstractKeyTuplewhich is private for some reason. - It feels wrong to use
KeyedTuplebecauseassert isinstance(row.one(), KeyedTuple)would break. - Right now
AbstractKeyedTupleis typed as generic, not sure if it brings anything to the table. - This feels wrong:
sqlalchemy.orm.query.Query[Tuple[main.User, builtins.int*, fallback=sqlalchemy.util._collections.AbstractKeyedTuple[Any]]] - On the other hand we should use a fallback for a tuple, otherwise keyed access will break.
- Right now keyed access results in
Any, I don't see how it would be possible to figure out keys properly in mypy.
Decided to use AbstractKeyedTuple as it aligns with actual type used. Should be ready to review. CLA signed.
It seems that this will produce Query[int] for something like session.query(Employee.id). I think however that sqlalchemy will also return a subclass of _LW for this query.
@libre-man just checked, you are correct. I'll fix it today/tomorrow.
Btw this should be good to go.
@ilevkivskyi If I implement your requested changes will we be good to go on this?
If I implement your requested changes will we be good to go on this?
I would first ask if original author @rafales is still interested in continuing the work on this. In either case a second round of review may be needed.
Hi @rafales! Do you think you're going to get a chance to finish this up any time soon?
Sorry guys, I lost track of GH for some time. I made @ckarnell a collaborator in the repo as he wants to finish this PR.
Thanks @rafales ! I had some free time when I was recently posting on this PR which I don't anymore, so I'm not sure I'll be able to get to this ASAP, but I'll try to some time soon!
Sorry for pinging, what is the status of this PR? Are you waiting on any input from me? Is anyone still willing to continue working on this?
I can see, there are some conflicts in this PR.
I think this is one of the single highest value things we can add to the stubs so I'd prefer if it didn't get closed, but I can't guarantee I'll have to to work on it soon.
@ilevkivskyi It actually seems like you had the most context based on your review, and getting it up to speed might be non-trivial because I noticed it doesn't appear to quite work with the new semantic analyzer. Is there any chance you could take a stab at finishing this?
Is there any chance you could take a stab at finishing this?
I may be busy next few weeks/months. If no one will finish this one during this time, then I can finish it myself.
Hi all, I've merged master and addressed review in another PR to @rafales ' fork here. I seem to be unable to request reviewers on that PR for some reason - @ilevkivskyi and @rafales , do you two mind reviewing it for me?
I will take a look at this before the end of the year.
I will take a look at this before the end of the year.
Actually, it looks like I will not have time to work on this, sorry.
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.