[PATCH v2 03/13] REST: Explicitly define fields

[Stephen Fincane]

On Mon, 2016-11-21 at 11:46 +1100, Daniel Axtens wrote:
> Hi Stephen,
> 
> > 
> > diff --git a/patchwork/api/check.py b/patchwork/api/check.py
> > index e88c62e..d24b0c6 100644
> > --- a/patchwork/api/check.py
> > +++ b/patchwork/api/check.py
> > @@ -63,8 +63,8 @@ class CheckSerializer(ModelSerializer):
> >      class Meta:
> >          model = Check
> >          fields = ('patch', 'user', 'date', 'state', 'target_url',
> > -                  'description', 'context',)
> > -        read_only_fields = ('date',)
> > +                  'context', 'description')
> > +        read_only_fields = ('date', )
> What's the rationale for thses changes? They look functionally
> equivalent. I first thought it was PEP8 but I don't see any warnings
> from the old rest_serializers file on my unpatched code base...

This is an artefact from the rebase onto master (where 'fields' had
already been defined). However, while the 'read_only_fields' change was
unnecessary (and is reverted in v3), the change to 'field' actually
makes sense as this affects the ordering of the output. In my mind, the
context field is of more significant and should come first in the
output. I can drop this hunk if you disagree, however.

> > 
> >  class CheckViewSet(PatchworkViewSet):
> > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py
> > index eea787d..bfd8fe3 100644
> > --- a/patchwork/api/patch.py
> > +++ b/patchwork/api/patch.py
> > @@ -40,34 +40,45 @@ class PatchListSerializer(ListSerializer):
> >  class PatchSerializer(HyperlinkedModelSerializer):
> >      mbox = SerializerMethodField()
> >      state = SerializerMethodField()
> > +    tags = SerializerMethodField()
> > +    headers = SerializerMethodField()
> > +    check = SerializerMethodField()
> >  
> > -    class Meta:
> > -        model = Patch
> > -        list_serializer_class = PatchListSerializer
> > -        read_only_fields = ('project', 'name', 'date',
> > 'submitter', 'diff',
> > -                            'content', 'hash', 'msgid')
> > -        # there's no need to expose an entire "tags" endpoint, so
> > we custom
> > -        # render this field
> > -        exclude = ('tags',)
> > +    def get_state(self, instance):
> > +        return instance.state.name
> > +
> > +    def get_mbox(self, instance):
> > +        request = self.context.get('request')
> > +        return request.build_absolute_uri(instance.get_mbox_url())
> > +
> > +    def get_tags(self, instance):
> > +        # TODO(stephenfin): I don't think this is correct - too
> > many queries
> > +        return [{'name': x.tag.name, 'count': x.count}
> > +                for x in instance.patchtag_set.all()]
> >  
> > -    def get_state(self, obj):
> > -        return obj.state.name
> > +    def get_headers(self, instance):
> > +        if instance.headers:
> > +            return
> > +        email.parser.Parser().parsestr(instance.headers, True)
> >  
> > -    def get_mbox(self, patch):
> > -        request = self.context.get('request', None)
> > -        return request.build_absolute_uri(patch.get_mbox_url())
> > +    def get_check(self, instance):
> > +        return instance.combined_check_state
> >  
> >      def to_representation(self, instance):
> >          data = super(PatchSerializer,
> > self).to_representation(instance)
> >          data['checks'] = data['url'] + 'checks/'
> > -        data['check'] = instance.combined_check_state
> > -        headers = data.get('headers')
> > -        if headers is not None:
> > -            data['headers'] =
> > email.parser.Parser().parsestr(headers, True)
> > -        data['tags'] = [{'name': x.tag.name, 'count': x.count}
> > -                        for x in instance.patchtag_set.all()]
> >          return data
> 
> These hunks seem to be doing things that are not in the commit
> message. Eyeballing the diff it seems like you:
>  - reordered the functions
>  - renamed some arguments
>  - added a comment
>  - made things line up better with DRF idiom.

Mostly the latter - I started using 'SerializerMethodField' instead of
overriding the 'to_representation' function. I've split this into a
separate patch for v3.

Stephen