Repository navigation
Added SQLAlchemy annotations - #1029
Conversation
Stiivi
commented
Mar 19, 2017
Stiivi
commented
- Made Connection and Engine sublcasses of Connectable (sqlalchemy Engine and Connection should be subclasses of Connectable #1018)
- Moved execute() from Connection to Connectable
- Made RowProxy a Mapping and removed Mapping inherited methods
- Made ResultProxy an Iterator of RowProxy
- Added most relevant methods for fetching of ResultProxy
- Added where(), group_by(), order_by() and limit() to Select
* Made Connection and Engine sublcasses of Connectable (#1018) * Moved execute() from Connection to Connectable * Made RowProxy a Mapping and removed Mapping inherited methods * Made ResultProxy an Iterator of RowProxy * Added most relevant methods for fetching of ResultProxy * Added where(), group_by(), order_by() and limit() to Select
| def close(self) -> None: ... | ||
| def __iter__(self) -> Iterator[RowProxy]: ... | ||
| def fetchall(self) -> Iterator[RowProxy]: ... | ||
| def fetchmany(self, size: int=None) -> Iterator[RowProxy]: ... |
There was a problem hiding this comment.
Better to write Optional[int] = ...
| def fetchmany(self, size: int=None) -> Iterator[RowProxy]: ... | ||
| def fetchone(self) -> Optional[RowProxy]: ... | ||
| def first(self) -> Optional[RowProxy]: ... | ||
| def scalar(self): ... |
There was a problem hiding this comment.
Very likely an union of DB API Type Objects. By brief search of this repo they don't seem to be defined anywhere to be shared except as internal part of sqlite and mysql.
Would it be ok to use Any with a comment reference to future union type?
This is also related to the RowProxy value type which should be of this union.
There was a problem hiding this comment.
I think returning Any explicitly with a comment is better because it shows that we've thought about it. (I'm hoping we can disallow missing types in typeshed at some point.)
There was a problem hiding this comment.
Added an issue, so we won't forget: #1037.
| class Select(HasPrefixes, HasSuffixes, GenerativeSelect): ... | ||
|
|
||
| class Select(HasPrefixes, HasSuffixes, GenerativeSelect): | ||
| def where(self, whereclause) -> Select: ... |
There was a problem hiding this comment.
Any way these can have more specific types?
There was a problem hiding this comment.
Sure, I plan to add them later once I get to the code that is using them.
| from typing import Any, Iterable, Iterator, List, Mapping, Optional, Tuple | ||
|
|
||
| # Dummy until I figure out something better. | ||
| class Connectable: |
There was a problem hiding this comment.
This class actually has numerous additional methods and it lives in interface.py, not base.py. I'm not sure how valuable it is to add those additional methods, but in general it might be worse to have incomplete stubs (missing important functions) than no stubs, because with incomplete stubs people will get false positive errors about missing functions. It sounds like that's currently happening with SQLAlchemy (#974).
There was a problem hiding this comment.
So what is your suggestion here? Add all the Connectable methods and move it into interface.pyi?
There was a problem hiding this comment.
That would be better yes. Not sure how many additional missing methods would have to be added.
There was a problem hiding this comment.
Ok, I'll prepare update to this PR.
* Created sqlalchemy.engine.result and moved ResultProxy and RowProxy there * Created sqlalchemy.engine.interfaces and moved Connectable there * Added non-deprecated methods to Connectable: connect, contextual_connect and scalar * Fixed return type of scalar() to Any
|
|
||
| # Note: The return type `Any` should be a DB API 2 value type once defined | ||
| # TODO: See #1037 | ||
| def scalar(self): ... |
There was a problem hiding this comment.
Can you put -> Any explicitly?
There was a problem hiding this comment.
Fixed. Had it in Connectable, thought the other was fixed already.
... had it in Connectable only.
|
Thanks! I'm going to merge this since it makes things better, but depending on what happens in #974 we may end up making bigger changes. |
2 participants
