dropbox / sqlalchemy-stubs

Mypy plugin and stubs for SQLAlchemy
Apache License 2.0
573 stars 101 forks source link

TypeDecorator.process_bind_param can return more than Optional[Text] #205

Open huonw opened 3 years ago

huonw commented 3 years ago

Currently, the process_bind_param method on TypeDecorator[_T] has signature:

https://github.com/dropbox/sqlalchemy-stubs/blob/8495c229cf8a31d75fcea36090e07c0e4b867605/sqlalchemy-stubs/sql/type_api.pyi#L95

Unfortunately, it looks like this is incorrect: I believe it can return anything that the underlying impl can accept. For instance, in SQLAlchemy's tests there's type decorators that return ints:

class MyNewIntType(types.TypeDecorator):
    impl = Integer

    def process_bind_param(self, value, dialect):
        return value * 10

    def process_result_value(self, value, dialect):
        return value * 10

    def copy(self):
        return MyNewIntType()

The process_bind_param return value should probably be loosened to match the Optional[Any] of its inverse operation process_result_value.

https://github.com/dropbox/sqlalchemy-stubs/blob/8495c229cf8a31d75fcea36090e07c0e4b867605/sqlalchemy-stubs/sql/type_api.pyi#L96

(This probably applies to process_literal_param too.)

watterso commented 3 years ago

I've run into this as well. My use case is a TypeDecorator that allows for storing unix timestamp as a float in the database and making it available in python as a utc datetime. This is done by making a TypeDecorator that takes a datetime in process_bind_param and returns a float with the situation flipped in process_result_value (it accepts and float and returns a datetime):

class FloatDateTime(TypeDecorator):
    impl = Float

    def process_bind_param(
        self, value: Optional[datetime.datetime], _
    ) -> Optional[float]:
        if value is None:
            return None
        return value.timestamp()

    def process_result_value(
        self, value: Optional[float], _
    ) -> Optional[datetime.datetime]:
        if value is None:
            return None

        return datetime.datetime.fromtimestamp(value, tz=datetime.timezone.utc)

I think the biggest downside to your proposed solution is that introducing Any into the mix adds ambiguity, I'm not expert enough in expressing python types to submit a code change (like you did in #206 ), but I think we want something would work like this:

huonw commented 3 years ago

@watterso I tried to do something like that, but I couldn't work out how express it on the TypeDecorator superclass. It'll take someone more experience with mypy/type hints than me to get it to work!

I think Any is a strict improvement over the current situation of str, because these functions are generally small and not reused, meaning the Any-ness doesn't propagate very fa (that is, there's a relatively small scope for mistakes).