clarkduvall / serpy

ridiculously fast object serialization
http://serpy.readthedocs.org/en/latest/
MIT License
960 stars 62 forks source link

Add a method for adding field after the definition of a serializer #34

Closed kinnou02 closed 8 years ago

kinnou02 commented 8 years ago

It's useful in case of circular dependency. Typically we have something like this:

ASerilizer(Serializer):
    b = BSerializer(many=True)

BSerializer(Serializer):
    a = ASerializer(many=True)

This isn't possible in python because BSerializer isn't yet define when we want to use it in ASerilizer. This Function give us the ability to solve this problem this way:

ASerilizer(Serializer):
    pass

BSerializer(Serializer):
    a = ASerializer(many=True)

ASerilizer.add_field('b', BSerializer(many=True)

Of course if the serialized objects reference themselves there will be some infinite loop.

If this proposition is fine for you I could add some documentation about it.

coveralls commented 8 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 0aab7128825c8a876ed0c694652b942bfea0aa70 on kinnou02:pr-add-field into a9ac2cf4e9a6d29d2d5e4ea62b0c0645243719da on clarkduvall:master.

clarkduvall commented 8 years ago

I could see this being useful in some situations, but there is a pretty easy workaround for this case:

class ASerializer(Serializer):
    b = MethodField()

    def get_b(self, obj):
        return BSerializer(obj.b, many=True).data

class BSerializer(Serializer):
    a = ASerializer(many=True)

The current implementation also has some problems with inheritance, for example this test will fail:

def test_add_field_inheritance(self):
    class ASerializer(Serializer):
        a = Field()

    class BSerializer(ASerializer):
        pass

    ASerializer.add_field('b', Field())

    a = Obj(a=5, b=2)
    self.assertEqual(BSerializer(a).data['a'], 5)
    self.assertEqual(BSerializer(a).data['b'], 2)

I think I would expect adding a field to the base class to also make it available to the child class. A correct implementation of this would have to iterate through all the child classes to add the field. I'm not sure if the extra complexity of this method is worth it, seeing that this is probably not a very common case, and there is a workaround for it.

What do you think? Does the workaround solve your issues?

kinnou02 commented 8 years ago

Thanks, I didn't think of using a method field, it's solve my issue. Updating every sub class will probably be tricky, I agree with you that this case isn't common, so I will close this PR.