-
Notifications
You must be signed in to change notification settings - Fork 21
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
Just modify the Django UUIDField for our 32 digit hex representation. #163
base: release-v0.6.x
Are you sure you want to change the base?
Conversation
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.
Probably a good move overall, but a few notes/concerns.
def prepare_value(self, value): | ||
if isinstance(value, uuid.UUID): | ||
return value.hex | ||
return 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.
This one doesn't seem to be captured in the parent class.
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.
(and is an important part of having it return the 32 char value rather than a UUID object, from what I recall)
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 maybe specifically for cases where someone sets it on there as a UUID object and then reads it back, without it going through the database yet)
But it could probably call to_python
here to be DRYer.
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.
prepare_value
is a form field method though, not a regular field: https://github.com/django/django/blob/stable/1.11.x/django/forms/fields.py#L129
I do wonder if that's where some of the original implementation errors came from, as it seems the original implementation here was a copy of the Django form field UUIDField: https://github.com/django/django/blob/stable/1.11.x/django/forms/fields.py#L1206 but then wrapped around the model CharField implementation instead of the form field.
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.
There is definitely something missing though, because during the serialization tests, it is returning as a UUID.
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 default Django serialization framework uses the value_to_string
method, which we seem to have not defined here.
See: https://github.com/django/django/blob/stable/1.11.x/django/db/models/fields/__init__.py#L834
Although we're then not using that in the serialize method: https://github.com/learningequality/morango/blob/release-v0.6.x/morango/models/core.py#L939
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.
Have now updated to add a value_from_object
override that properly coerces the value to a 32 digit hex string.
value = super(UUIDField, self).to_python(value) | ||
|
||
if connection.features.has_native_uuid_field: | ||
return value | ||
return value.hex |
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'm now not seeing a difference between this and the parent class implementation, other than the use of super
, which doesn't seem necessary, since to_python
isn't defined in this class, so it'll use the super's anyway. So I'm guessing this can be removed?
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.
It is, see below.
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.
Looks good to me, once it's been tested within Kolibri's test suite as well!
Summary
TODO
Reviewer guidance
Don't think I've missed anything but I'd appreciate @jamalex's eyes to be sure.
Issues addressed
Fixes #89