Various updates to sqlalchemy.sql typings #46
Conversation
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks for the PR! New types look good. I have some comments below.
There is also a bunch of new errors in our internal codebase where Mapping[Union[str, ColumnClause[Any]], Any] is expected (in various methods), but Dict[Column[Any], Any] is given. This is caused by invariance of Mapping in key type. I am not sure what is the best way to proceed here. Probably we can come back to this problem later. Could you please open an issue for this?
| def compare_values(self, x, y): ... | ||
| def comparator_factory(self) -> Type[Any]: ... | ||
| def type_engine(self, dialect: Dialect) -> TypeEngine[Any]: ... | ||
| def load_dialect_impl(self, dialect: Dialect) -> TypeEngine[Any]: ... |
There was a problem hiding this comment.
This creates a bunch of errors because some subtypes in our code are annotated as accepting defaults.DefaultDialect, could you please double-check here and in the other methods below?
(I might well be that we just need to fix annotations in our codebase.)
There was a problem hiding this comment.
Dialect is the "interface" and DefaultDialect is the default implementation (which inherits from Dialect). From what I can tell, dialects can technically inherit from Dialect and implement all the things they need to, but the dialects in sqlalchemy.dialects all inherit from DefaultDialect. My gut feeling is to leave it as Dialectand build up that object's properties with the properties included in docstring. I can do that in this PR if you'd like.
There was a problem hiding this comment.
Let's just leave it as is then. We can come back to this later if there will be problems.
There was a problem hiding this comment.
Do you want me to add the properties to the Default interface? If so, should the properties be abstract?
There was a problem hiding this comment.
Not in this PR. Maybe at some point later. I think this is relatively low priority.
sqlalchemy-stubs/sql/sqltypes.pyi
Outdated
| def python_type(self) -> Type[list]: ... | ||
| def compare_values(self, x: _T, y: _T) -> bool: ... | ||
| def python_type(self) -> Type[List[_T]]: ... | ||
| def compare_values(self, x: Any, y: Any) -> bool: ... |
There was a problem hiding this comment.
Why use Any here, could you please add a comment?
There was a problem hiding this comment.
The function just does x == y, so it can take any values. I'll add a comment.
There was a problem hiding this comment.
Hm, sometimes mypy prohibits things that work at runtime. For example:
def add(x: int, y: int) -> int:
x + yalso works for strings before one adds types. Anyway, if you are sure it is intended to be used with arbitrary objects, then object instead of Any would be better.
There was a problem hiding this comment.
I traced this to where it's used and it looks to be used within sqlalchemy.orm.strategies and sqlalchemy.orm.attributes where if compare_function is not provided, operator.eq is used and operator.eq is def eq(a: Any, b: Any) -> Any: .... I'm reasonably confident that Any, Any is fine here.
|
I went ahead and made sure that the |
sqlalchemy-stubs/sql/selectable.pyi
Outdated
| def __getattr__(self, attr: str) -> Any: ... | ||
|
|
||
| _ValuesType = Optional[Union[Mapping[str, Any], | ||
| Mapping[ColumnClause[Any], Any], |
There was a problem hiding this comment.
I think you might need to also add Mapping[Column[Any], Any] to the union, otherwise the errors I found in our codebases will remain (since Dict[Column[Any], Any] is not a subtype of this because Mapping is invariant in key type).
There was a problem hiding this comment.
Oh, you said invariant and I interpreted it as covariant. Now I understand the issue. Maybe we should use Mapping[Any, Any] for now and then possibly create our own mapping type that is covariant.
There was a problem hiding this comment.
OK, It looks like Mapping[Any, Any] is the best option for now. Could you please also add a comment explaining why we don't use "more precise" types?
|
Thanks for the updates! This is almost ready to go, I left few small comments. |
|
I pushed the fixes for |
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
LGTM! I will merge when tests pass.
2 participants
No description provided.