-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add support for deserialization #2
base: master
Are you sure you want to change the base?
Conversation
- Added support for read-only fields - Renamed `.data` to `.representation` (and added `.internal_value`) for better clarity - Renamed `.to_value` to `.to_representation` (and added `to_internal_value`) for better clarity TBD update documentation, fix benchmarks, update benchmarks Fixes clarkduvall#1
thanks for the pr! will take a look tonight at it |
quick high level comment: |
if method_name is None: | ||
method_name = 'get_{0}'.format(serializer_field_name) | ||
return getattr(serializer_cls, method_name) | ||
return getattr(serializer_cls, method_name, 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 would rather not specify the default to getattr and have these methods fail if method_name doesn't exist
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.
+1
It looks like DRF uses update/create/save for deserialization: http://www.django-rest-framework.org/api-guide/serializers/#saving-instances I'm thinking something like that would be nicer than the |
|
||
def __init__(self, obj=None, many=False, **kwargs): | ||
def __init__(self, obj=None, data=None, many=False, **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.
what do you think about allowing an instance of a serializer to either serialize or deserialize, but not both. This could have one obj parameter like it was before, and it will either fill out self._representation or self._internal_value, and throw an exception if it tries to do both.
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 particularly like overriding parameters that way: Serializer(instance_or_data=foo)
. But - I agree that by making a serializer do only one of serialization or deserialization would make it faster (eg. don't need to compile "write fields")
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.
yeah, I like your proposal below about using instance
for serialization and data
for deserialization, lets go with that
would be cool to add some benchmarks comparing the deserialization speed to DRF and Marshmallow, but that doesn't have to be in this PR |
Anything that'd make it easy to use as an alternative serializer implementation for REST framework would get a big +1 from me 😄 |
@clarkduvall @tomchristie Agreed that interface should be a drop-in replacement of DRF. How about:
Agreed on the |
@maroux I like the I think it would be more flexible to have a function instead of just specifying the class for deserialization in case some extra massaging of the data has to be done before returning the instance. So if we had a def make_instance(self, data):
return SomeClass(**data)
@property
def instance(self):
if self._instance is None:
self._instance = self.make_instance(self.to_internal_value(self._data))
return self._instance by default make_instance could just be a noop |
@clarkduvall So the problem with How about this:
and
I'm not sure if there is value is setting a default value of |
Actually, disregard that suggestion, the |
New API - `.data` and `.deserialized_value` Making `cls` a kwarg in `Serializer` instead of attribute
@clarkduvall What do you think about #3? I'm now inclined to go back to the |
Yeah I think as more features are added the |
@@ -79,11 +79,11 @@ Simple Example | |||
z = serpy.Field() | |||
|
|||
f = Foo(1) | |||
FooSerializer(f).data | |||
FooSerializer(f).representation |
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.
change these back to .data
I still want a |
@clarkduvall I'm confused, wouldn't |
yep, by default it would be a no-op unless you override it to return a class, see marshmallows implementation: https://github.com/marshmallow-code/marshmallow/blob/c4330c80073811bc175ffead89fe3cdc91797421/marshmallow/schema.py#L587-L595 For example to override it for a Django User model: def make_object(self, data):
return User(**data) then when you do serializer.deserialized_data it will return a User object |
I see what you mean. What's your thought on keeping it no-op in the default implementation, but have another serializer that lets you pass in |
@maroux can you give me an example of what you mean? |
I just came across this great library. Deserialization would be a big win - I can't really contribute much at this point besides my gratitude. |
I would be really thankful if deserialization compatible with DRF is implemented. |
I'm using serpy in one of my projects and now I have to use DRF for deserialization and serpy for serialization. Having to define the same schema twice is rather unconvenient. Are there any plans when deserialization is going to be supported? +1 for the make_object method rather than calling cls() as it allows for structures like |
I don't have a lot of time to work on this now, but if someone wants to implement this (or @maroux wants to continue this PR) using the make_object interface that was discussed earlier, I would definitely take a look at it. |
# If to_value isn't a method, it must have been overriden. | ||
if not isinstance(to_value, types.MethodType): | ||
def to_internal_value(self, data): | ||
return data |
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.
Wouldn't you still want to cast it back to int
?
|
||
:param instance: The object or objects to serialize. | ||
:param data: The data to deserialize. | ||
:param klass: The class for instantiating the deserialized object |
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 doesn't seem to be used at all
.data
to.representation
(and added.internal_value
) for better clarity.to_value
to.to_representation
(and addedto_internal_value
) for better clarityTBD update documentation, fix benchmarks, update benchmarks
Fixes #1