-
Notifications
You must be signed in to change notification settings - Fork 14
Add LocalizedBooleanField #93
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
Conversation
| def __init__(self, *args, **kwargs): | ||
| super().__init__(*args, **kwargs) |
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.
Useless, this is the default behavior.
| if re.match("False", local_value, re.IGNORECASE): | ||
| local_value = False | ||
| elif re.match("True", local_value, re.IGNORECASE): | ||
| local_value = True |
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.
Bit overkill to use regex for this when .lower() will do the job.
| # if we were used in an expression somehow then it might be | ||
| # that we're returning an individual value or an array, so | ||
| # we should not convert that into an :see:LocalizedBooleanValue | ||
| if not isinstance(db_value, LocalizedValue): | ||
| return db_value |
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.
The comment explains what we're doing, not why.
| elif re.match("True", local_value, re.IGNORECASE): | ||
| local_value = True | ||
| else: | ||
| local_value = 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.
This should raise an error instead of silently ignoring the problem.
| for lang_code, _ in settings.LANGUAGES: | ||
| local_value = value.get(lang_code, None) | ||
|
|
||
| if isinstance(local_value, str): |
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 local_value is not a string (unlikely), this check will just swoop the error under the carpet instead of loudly failing.
| for lang_code, _ in settings.LANGUAGES: | ||
| local_value = prepped_value[lang_code] | ||
|
|
||
| if local_value is not None and local_value not in ("False", "True"): |
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.
Why would the values already be strings at this point?
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 the values get converted from bool to str during this step prepped_value = super().get_prep_value(value)
| class LocalizedBooleanValue(LocalizedValue): | ||
| def __bool__(self): | ||
| """Gets the value in the current language as a boolean,""" | ||
| value = self.translate() |
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.
Shouldn't self.translate() return a boolean like the other value classes?
No description provided.