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

Improve PostgreSQL's UUID support #95

Open
wants to merge 1 commit into
base: master
from

Conversation

@bochecha
Copy link

bochecha commented Jul 25, 2019

SQLAlchemy allows setting UUID columns to either a string representation
of a UUID (e.g '46260785-9b7e-4a59-824f-af994a510673') or to a Python
uuid.UUID object.

Fixes #94

SQLAlchemy allows setting UUID columns to either a string representation
of a UUID (e.g '46260785-9b7e-4a59-824f-af994a510673') or to a Python
uuid.UUID object.

Fixes #94
@rafales
Copy link
Contributor

rafales commented Jul 26, 2019

While this fixes your particular use case - it will break stuff for other people. Now all UUID fields on a model will be an union of str and UUID and client code will need to handle those cases.

I think the right solution here is to extend mypy plugin.
Basically what needs to be done is to hook into get_function_hook and return proper type (Instance with type argument set to str or UUID ) based on as_uuid argument.

@JukkaL
Copy link
Collaborator

JukkaL commented Jul 26, 2019

I wonder if it would be possible to use an overloaded function with literal types to fix this? Maybe something like subprocess.Popen: https://github.com/python/typeshed/blob/master/stdlib/3/subprocess.pyi#L809

@rafales
Copy link
Contributor

rafales commented Jul 26, 2019

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi commented Aug 7, 2019

I think we should try what @JukkaL proposes, using a union in this context may cause many false positives.

@bochecha
Copy link
Author

bochecha commented Aug 9, 2019

I wonder if it would be possible to use an overloaded function with literal types to fix this? Maybe something like subprocess.Popen: https://github.com/python/typeshed/blob/master/stdlib/3/subprocess.pyi#L809

It seems I took too long to reply and this line moved? Otherwise I don't see how line 809 of current master relates to this. 😕

I guess this is what you had in mind? https://github.com/python/typeshed/blob/3ad3ed82c75b4fb82f71f5500b50f18a98f12f49/stdlib/3/subprocess.pyi#L809

I just tried the following:

diff --git a/sqlalchemy-stubs/dialects/postgresql/base.pyi b/sqlalchemy-stubs/dialects/postgresql/base.pyi
index d910e5c..2c6917f 100644
--- a/sqlalchemy-stubs/dialects/postgresql/base.pyi
+++ b/sqlalchemy-stubs/dialects/postgresql/base.pyi
@@ -1,8 +1,9 @@
 from ... import schema
 from ...engine import default, reflection
 from ...sql import compiler, expression, sqltypes, type_api
-from typing import Any, Optional, Set, Type, Text, Pattern, Dict
+from typing import Any, Optional, Set, Type, Text, Pattern, Dict, overload
 from datetime import timedelta
+import uuid
 
 from sqlalchemy.types import INTEGER as INTEGER, BIGINT as BIGINT, SMALLINT as SMALLINT, VARCHAR as VARCHAR, \
     CHAR as CHAR, TEXT as TEXT, FLOAT as FLOAT, NUMERIC as NUMERIC, \
@@ -66,12 +67,20 @@ class BIT(sqltypes.TypeEngine[str]):
     def __init__(self, length: Optional[int] = ..., varying: bool = ...) -> None: ...
 PGBit = BIT
 
+@overload
 class UUID(sqltypes.TypeEngine[str]):
     __visit_name__: str = ...
     as_uuid: bool = ...
     def __init__(self, as_uuid: bool = ...) -> None: ...
     def bind_processor(self, dialect: Any): ...
     def result_processor(self, dialect: Any, coltype: Any): ...
+@overload
+class UUID(sqltypes.TypeEngine[uuid.UUID]):
+    __visit_name__: str = ...
+    as_uuid: bool = ...
+    def __init__(self, as_uuid: bool = ...) -> None: ...
+    def bind_processor(self, dialect: Any): ...
+    def result_processor(self, dialect: Any, coltype: Any): ...
 PGUuid = UUID
 
 class TSVECTOR(sqltypes.TypeEngine[str]):

However that doesn't work (same error message as #94). It seems @overload only works for functions and methods, not classes.

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi commented Aug 9, 2019

You need to overload the constructor, not the class itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.