| Message ID | 1479574288-24171-4-git-send-email-stephen@that.guru |
|---|---|
| State | Superseded |
| Headers | show |
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
On 11/19/2016 10:51 AM, Stephen Finucane wrote: > While a little more verbose, this ensures the fields that are exposed > by the API remain consistent. This also resolves an issues with > django-rest-framework 3.5, which expects either 'fields' or 'excludes' > to be defined on all serializers. > > The Meta class for the Project and Patch serializers are moved to the > bottom of the serializers for consistency's sake. > > Signed-off-by: Stephen Finucane <stephen@that.guru> > Cc: Andy Doan <andy.doan@linaro.org> I had similar questions to Daniel. However, the general idea is an improvement.
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
> 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. I think the best of both worlds would be to drop the change to read_only_fields and keep the change to field. >> 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. Many thanks. Regards, Daniel
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', ) 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 + 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') + class PatchViewSet(PatchworkViewSet): permission_classes = (PatchworkPermission,) diff --git a/patchwork/api/person.py b/patchwork/api/person.py index 9fda027..7507fe5 100644 --- a/patchwork/api/person.py +++ b/patchwork/api/person.py @@ -27,7 +27,7 @@ from patchwork.models import Person class PersonSerializer(HyperlinkedModelSerializer): class Meta: model = Person - fields = ('email', 'name', 'user') + fields = ('url', 'name', 'email', 'user') class PeopleViewSet(PatchworkViewSet): diff --git a/patchwork/api/project.py b/patchwork/api/project.py index ea09acc..17f4b3c 100644 --- a/patchwork/api/project.py +++ b/patchwork/api/project.py @@ -25,10 +25,6 @@ from patchwork.models import Project class ProjectSerializer(HyperlinkedModelSerializer): - class Meta: - model = Project - exclude = ('send_notifications', 'use_tags') - def to_representation(self, instance): data = super(ProjectSerializer, self).to_representation(instance) data['link_name'] = data.pop('linkname') @@ -36,6 +32,11 @@ class ProjectSerializer(HyperlinkedModelSerializer): data['list_id'] = data.pop('listid') return data + class Meta: + model = Project + fields = ('url', 'name', 'linkname', 'listid', 'listemail', 'web_url', + 'scm_url', 'webscm_url') + class ProjectViewSet(PatchworkViewSet): permission_classes = (PatchworkPermission,) diff --git a/patchwork/api/user.py b/patchwork/api/user.py index aa788b8..1da90ac 100644 --- a/patchwork/api/user.py +++ b/patchwork/api/user.py @@ -27,9 +27,7 @@ from patchwork.api import PatchworkViewSet class UserSerializer(HyperlinkedModelSerializer): class Meta: model = User - exclude = ('date_joined', 'groups', 'is_active', 'is_staff', - 'is_superuser', 'last_login', 'password', - 'user_permissions') + fields = ('url', 'username', 'first_name', 'last_name', 'email') class UserViewSet(PatchworkViewSet):
While a little more verbose, this ensures the fields that are exposed by the API remain consistent. This also resolves an issues with django-rest-framework 3.5, which expects either 'fields' or 'excludes' to be defined on all serializers. The Meta class for the Project and Patch serializers are moved to the bottom of the serializers for consistency's sake. Signed-off-by: Stephen Finucane <stephen@that.guru> Cc: Andy Doan <andy.doan@linaro.org> --- patchwork/api/check.py | 4 ++-- patchwork/api/patch.py | 49 +++++++++++++++++++++++++++++------------------- patchwork/api/person.py | 2 +- patchwork/api/project.py | 9 +++++---- patchwork/api/user.py | 4 +--- 5 files changed, 39 insertions(+), 29 deletions(-)