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

Fix type of min_value and max_value on DecimalField #951

Merged
merged 4 commits into from
May 10, 2022

Conversation

ljodal
Copy link
Contributor

@ljodal ljodal commented May 9, 2022

These should at the very least allow Decimals. Technically you can send in anything that's comparable to a Decimal, but I'm not sure if it makes sense to allow floats. Could allow both ints and Decimals I guess?

In the Django codebase these arguments are sent to MinValueValidator and MaxValueValidator which just does a < b/a > b.

These should at the very least allow Decimals. Technically you can send
in anything that's comparable to a Decimal, but I'm not sure if it makes
sense to allow floats. Could allow both ints and Decimals I guess?
@@ -125,8 +125,8 @@ class DecimalField(IntegerField):
def __init__(
self,
*,
max_value: Optional[int] = ...,
min_value: Optional[int] = ...,
max_value: Optional[Decimal] = ...,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>>> from decimal import Decimal
>>> Decimal(1.5) > 1.2
True
>>> Decimal(1.5) > 1
True

Let's include int and float

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I've included them now. Kinda disagree about floats though, because the imprecise nature of floats. If you've decided to use a decimal field I assume you want accuracy, even though a float technically works.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

django-stubs/forms/fields.pyi Outdated Show resolved Hide resolved
django-stubs/forms/fields.pyi Outdated Show resolved Hide resolved
@sobolevn sobolevn merged commit 7d84e54 into typeddjango:master May 10, 2022
@ljodal ljodal deleted the decimal-field-min-max-value branch May 10, 2022 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants