-
-
Notifications
You must be signed in to change notification settings - Fork 400
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
IntField: add MinMaxValidator #1868
base: develop
Are you sure you want to change the base?
Conversation
subclassing IntField and setting GE and/or LE will directly be reflected in the MinMaxValidator and the constraints property
MyISAM does not support transactions
CodSpeed Performance ReportMerging #1868 will not alter performanceComparing Summary
|
Pull Request Test Coverage Report for Build 13053921297Details
💛 - Coveralls |
maybe entering the loop in validate(value) causes those performance issues...
self.le = le | ||
|
||
def to_db_value(self, value: Any, instance: "Union[Type[Model], Model]") -> Any: | ||
if value is not None: |
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 think we should be adding a validator instead of customizing to_db_values
similarly how it is done in CharField
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.
that's what I did first, but it adds far too much overhead decreasing the benchmarks like 10% (see the edit history of the comment of codspeed..)
I think that's because it will actually enter the for each loop in self.validate instead of doing nothing. Also I think that overriding to_do_value actually increases speed because the interpreter does not have to go that much up the MRO.
def __init__( | ||
self, | ||
primary_key: Optional[bool] = None, | ||
ge: Optional[int] = None, |
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.
Extending the field configuration with ge and le introduces the second way of setting up validation. I think we should have a single way of doing it to keep things simple - through validators. If some one needs specific validation, they can add a validator. So I suggest to remove these options from the constructor.
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.
ge and le are currently baked in in the constraints property. Instead of that I wanted them to be customizable, a bit like with max_char in CharField where this is reflected during validation and also by the constraints property.
@@ -269,7 +269,7 @@ def validate(self, value: Any) -> None: | |||
""" | |||
for v in self.validators: | |||
if self.null and value is None: | |||
continue | |||
return |
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 don't think this is correct.
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.
If you have like ten validators in self.validators, you will always have to perform the check whether the field may be null and if the given value is None/null. Both informations do not change during itering other the validators. So instead of performing the same check over and over again, we can instead directly say that nothing else will happen here and that the value is valid.
Description
Before sending anything to the DB, IntField values are checked beforehand now. As far as I could test, the added overhead is not much, lets see what Codspeed says.
Will add documentation later.
Motivation and Context
Using sqlite, you could send values greater than defined in
IntField.constraints.get('le')
to the DB and nothing was raised. If you for example want to migrate to another DBMS, you have a problem.Also, have a look at the Issue I raised: #1853
How Has This Been Tested?
Existent tests and new tests
Checklist: