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

[Daniel Axtens]

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...

>  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.

Is that correct?

>  
> +    class Meta:
> +        model = Patch
> +        list_serializer_class = PatchListSerializer
> +        fields = ('url', 'project', 'msgid', 'date', 'name', 'commit_ref',
> +                  'pull_url', 'state', 'archived', 'hash', 'submitter',
> +                  'delegate', 'mbox', 'check', 'checks', 'tags', 'headers',
> +                  'content', 'diff')
> +        read_only_fields = ('project', 'name', 'date', 'submitter', 'diff',
> +                            'content', 'hash', 'msgid')
> +

The other hunks look good.

Regards,
Daniel