Skip to content

Data type for Numeric column should be Decimal, not float #131

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

Closed
martinstein opened this issue Dec 11, 2019 · 8 comments
Closed

Data type for Numeric column should be Decimal, not float #131

martinstein opened this issue Dec 11, 2019 · 8 comments

Comments

@martinstein
Copy link
Contributor

martinstein commented Dec 11, 2019

The official SQLAlchemy documentation for the column type Numeric says:

This type returns Python decimal.Decimal objects by default, unless the Numeric.asdecimal flag is set to False, in which case they are coerced to Python float objects.

https://docs.sqlalchemy.org/en/13/core/type_basics.html#sqlalchemy.types.Numeric

Currently this is specified in sqltypes.pyi as TypeEngine[float] but by default the inner data type for Numeric should be Decimal, not float. That would be more in line with

  • what the SQLAlchemy documentation says.
  • how SQLAlchemy actually behaves.

The current approach reports typing errors even though the code is correct (due to the float/Decimal confusion) . Example to reproduce:

from decimal import Decimal
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy import Column, Integer, Numeric

Base = declarative_base()


class Transaction(Base):
    __tablename__ = 'transactions'
    id = Column(Integer, primary_key=True)
    amount = Column(Numeric, nullable=False)


user = User(id=42, amount=Decimal(42))

This code is 100% correctly typed, but since the sqlalchemy-stubs use float instead of Decimal we receive a typing-error message that we shouldn't get:

mypy-sa-test.py:14: error: Incompatible type for "amount" of "User" (got "Decimal", expected "float")
@msullivan
Copy link

That seems plausible. Do you want to submit a PR?

I am worried about whether this will cause problems if a lot of existing code assumes float already, though? But if that is wrong, maybe that's a good thing

@martinstein
Copy link
Contributor Author

Yes, happy to submit a PR and I agree that if existing code assumes float, fixing this will likely help those users to uncover a wrong type assumption.
I'm wondering: Is there a way to set up the type-stub so the type of the field depends on the value of the asdecimal argument of the Column? (which is True by default)

@dbanty
Copy link
Contributor

dbanty commented Jan 8, 2020

Any word on when this will make it into a release? Would save me quite a few casts and # type: ignores 😸

@ave2me
Copy link

ave2me commented Jul 9, 2020

I'm also very interested if there any news on when this fix will be included in a release.

@raychan
Copy link

raychan commented Aug 27, 2020

Looks like this issue is fixed, can we release the fix?

@nick-bigger
Copy link

I'm also wondering when this fix will go in?

@ilevkivskyi
Copy link
Contributor

I am going to make a release later today. Please test with current master to check if there are any issues.

@ilevkivskyi
Copy link
Contributor

(closing the issue since IIUC this is fixed in master)

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

No branches or pull requests

7 participants