Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
feat: enforce Decimal type in min_value and max_value arguments of DecimalField#8972
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
auvipy left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I am wondering if this enforcement create some noise or might break existing codes which were not using float and decimal in right separation? in that case should we also improve the docs if / where necessary
IsmaelJS commentedMay 4, 2023
I think the change may "break" the code but actually is fixing bugs on projects in which those input arguments are not decimal types. |
kevin-brown commentedMay 6, 2023
My preference would be to trigger a warning for now and put this through the deprecation cycle. We previously allowed floats, ints, and Decimals and now we are only allowing Decimal. I definitely agree with the goal, but breaking it immediately is probably not the right move. |
auvipy commentedMay 7, 2023
I think we should keep supporting Int, float & decimal all. and we should mostly align with django first |
IsmaelJS commentedMay 7, 2023
I agree with that a first step should be warn the situation instead of break. The point of@auvipy about aligning with Django is also interesting, because actually a [Min|Max]ValueValidator with any number type can be added to a models.DecimalField. Thus, do you agree guys to only leave a warning message in this PR? |
auvipy commentedMay 7, 2023
oh I see I miss read it. decimalField should support decimals only. but as it might break existing codes, we should only start with the warning for now :) |
auvipy commentedMay 7, 2023
we should move towards the path of django but also provide the users a graceful way to fix their code |
IsmaelJS commentedMay 7, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@auvipy done! I also included a test. |
terencehonles commentedApr 5, 2024
It looks like this finally got released, so sorry for the delay, but this really should have used warnings instead of logging a warning. We run our tests flagging warnings as errors, but I just happened to see this in the logs and it was less obvious than it could have been to track down where this was coming from. I assume this is really only a problem when using |
Description
Fixes issue#8963