-
Notifications
You must be signed in to change notification settings - Fork 771
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 1275: Make MaxValueValidator default an integer #1276
base: main
Are you sure you want to change the base?
Fix 1275: Make MaxValueValidator default an integer #1276
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1276 +/- ##
=======================================
Coverage 99.44% 99.44%
=======================================
Files 15 15
Lines 1265 1265
=======================================
Hits 1258 1258
Misses 7 7
Continue to review full report at Codecov.
|
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.
Hi.
I deliberately don't want to add a kwarg. It complicates the interface for all users, only 1 in a million of which will ever need to change it. Create a subclass if that's you. (Such would need docs and tests.)
Can you add a test case showing the problem case this change would resolve? (Thinking through trapping the signal there...)
Cheers.
Hi, thanks for the input!
Removed the kwarg again, just thought it might come in handy for some people - no feelings in either way from my side.
I added a test-case that activates the Decimal Trap first. The idea is to avoid some - for the user - strange side-effects of float/Decimal/int-comparisons (https://docs.python.org/3/library/decimal.html#floating-point-notes). Does this make sense for you? |
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.
Super. Thanks for the update. Let me have a play, but looks good.
@@ -362,7 +362,7 @@ def get_max_validator(self): | |||
""" | |||
Return a MaxValueValidator for the field, or None to disable. | |||
""" | |||
return MaxValueValidator(1e50) | |||
return MaxValueValidator(int(1e50)) |
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 just want to have a look at the order of validation in the field, but maybe this should be a decimal already...
(i.e. did we already run the value from the widget through to_python
, giving us a decimal in the value < limit
check...)
This should fix the issue #1275 by making the default value an integer and providing the option to set the max value per filter instance