Bug#824912: tracker.d.o: add an API for action items
Hi,
On Wed, 14 Dec 2016, efkin wrote:
> I decided to keep DRF 2.4.3. But i'm happy to change it if you prefer.
> I have no big reasons to prefer one or the other one, so i fallback on
> the fact that if it's on stable i go for it.
stretch is coming in a few months and we are not using Django from stable
either, so I would suggest to use directly DRF 3.4.
> > Reported-by: Paul Wise <pabs@debian.org>
>
> Done.
You are still missing: s/_/-/
> Btw sorry for the short description of my last email, i try to
> compensate it now:
What about using the API self-documentation feature?
http://www.django-rest-framework.org/topics/documenting-your-api/#self-describing-apis
That will also force you to choose good names to your views.
> diff --git a/distro_tracker/api/tests.py b/distro_tracker/api/tests.py
> index 7ce503c..255bbb8 100644
> --- a/distro_tracker/api/tests.py
> +++ b/distro_tracker/api/tests.py
> @@ -1,3 +1,104 @@
> -from django.test import TestCase
> +# Copyright 2014-2016 The Distro Tracker Developers
Copyright 2016 only for new code. :) Applies on all files.
> --- /dev/null
> +++ b/distro_tracker/api/tracker_urls.py
> @@ -0,0 +1,29 @@
> +# Copyright 2014-2016 The Distro Tracker Developers
> +# See the COPYRIGHT file at the top-level directory of this distribution and
> +# at https://deb.li/DTAuthors
> +#
> +# This file is part of Distro Tracker. It is subject to the license terms
> +# in the LICENSE file found in the top-level directory of this
> +# distribution and at https://deb.li/DTLicense. No part of Distro Tracker,
> +# including this file, may be copied, modified, propagated, or distributed
> +# except according to the terms contained in the LICENSE file.
> +
> +from django.conf.urls import url
> +
> +from .views import ActionItemListAPIView
> +from .views import ActionItemDetailAPIView
> +
> +
> +urlpatterns = [
> + url(r'^api/v1/action-items/?$',
> + ActionItemListAPIView.as_view(),
> + name='dtracker-api-v1-action-items'),
> + url(r'^api/v1/action-items/(?P<pk>[0-9]+)/?$',
> + ActionItemDetailAPIView.as_view(),
> + name='dtracker-api-v1-action-items'),
> +]
Please don't use the same "name" for both URL. Use something like
"...-action-item-list" and "...-action-item-detail".
Also, after having looked at the DRF doc, we should not hardcode the
versioning at this level, we should rather use NamespaceVersioning:
http://www.django-rest-framework.org/api-guide/versioning/#namespaceversioning
> +class ActionItemListAPIView(generics.ListAPIView):
Shouldn't we merge both objects with a single ReadOnlyModelViewSet?
http://www.django-rest-framework.org/api-guide/viewsets/#readonlymodelviewset
Also you should name the object "ActionItemListView" as the online documentation
only strips the "View" suffix.
> + """
> + List all ActionItem instances.
> + """
Here you must explain the GET query parameter that we support.
> + def get_queryset(self):
> + queryset = ActionItem.objects.all()
> + package_name = self.request.GET.get('package_name', None)
package_name is a required abstraction at the database level but it makes
no sense for end users. I would just use "package" as the query parameter.
Also, one of the early design decision seems to be whether we use an
hyperlinked serializer or not. I don't have a strong opinion on that but it
makes it easier to navigate interactively in the data through the browser. So
unless we have a reason not to, I believe it would be nice to use it:
http://www.django-rest-framework.org/tutorial/5-relationships-and-hyperlinked-apis/#hyperlinking-our-api
But even if we use the hyperlinked version of the serializer, I would still
include the "id" attribute everywhere.
Cheers,
--
Raphaël Hertzog ◈ Debian Developer
Support Debian LTS: http://www.freexian.com/services/debian-lts.html
Learn to master Debian: http://debian-handbook.info/get/
Reply to: