[RFC 02/11] REST: Add base configuration hooks for a REST API

[Andy Doan]

On 05/09/2016 08:15 AM, Finucane, Stephen wrote:
> On 15 Apr 13:23, Andy Doan wrote:
>> This adds the ability to expose a REST API based on the Django REST
>> framework project. Since this project isn't packaged in most current
>> distributions, we ensure that its both installed and enabled before
>> trying to use it.
>>
>> Signed-off-by: Andy Doan <andy.doan at linaro.org>
>> Inspired-by: Damien Lespiau <damien.lespiau at intel.com>
>
> Finally getting around to reviewing this. Sorry for the wait - I've
> been very busy :S
>
> I've a few nits below, but nothing the v2 should be mergeable.
>
> Stephen
>
>> ---
>>   patchwork/settings/base.py  | 11 +++++++++++
>>   patchwork/urls.py           |  7 +++++++
>>   patchwork/views/rest_api.py | 28 ++++++++++++++++++++++++++++
>>   requirements-test.txt       |  1 +
>>   4 files changed, 47 insertions(+)
>>   create mode 100644 patchwork/views/rest_api.py
>>
>> diff --git a/patchwork/settings/base.py b/patchwork/settings/base.py
>> index 2f81d4b..909ae4b 100644
>> --- a/patchwork/settings/base.py
>> +++ b/patchwork/settings/base.py

>
>> +    import rest_framework  # NOQA
>> +    INSTALLED_APPS.append('rest_framework')
>
> nit: The rest of the file uses arithmetic notation, i.e.
> "+= ['rest_framework']". Using '.append' is techincally faster, but
> unless we change every other entry then I would favour consistency.

That's fine, I was just copying the pattern 15 lines down where it does:

  MIDDLEWARE_CLASSES.append(

>> +except ImportError:
>> +    pass
>> +
>>   # HTTP
>>
>>   MIDDLEWARE_CLASSES = [
>> @@ -148,6 +156,9 @@ NOTIFICATION_FROM_EMAIL = DEFAULT_FROM_EMAIL
>>   # Set to True to enable the Patchwork XML-RPC interface
>>   ENABLE_XMLRPC = False
>>
>> +# Set to True to enable the Patchwork REST API
>> +ENABLE_REST_API = False
>> +
>>   # Set to True to enable redirections or URLs from previous versions
>>   # of patchwork
>>   COMPAT_REDIR = True
>> diff --git a/patchwork/urls.py b/patchwork/urls.py
>> index f3fdc5b..c5cabe5 100644
>> --- a/patchwork/urls.py
>> +++ b/patchwork/urls.py
>> @@ -138,6 +138,13 @@ if settings.ENABLE_XMLRPC:
>>               name='pwclientrc'),
>>       ]
>>
>> +if settings.ENABLE_REST_API:
>> +    if 'rest_framework' not in settings.INSTALLED_APPS:
>> +        raise RuntimeError(
>> +            'djangorestframework must be installed to enable the REST API.')
>
> nit: Rather than raising a runtime error, why not just let the
> ImportError bubble up?

The thing is that the ImportError isn't really a problem unless 
settings.ENABLE_REST_API was enabled. Givent the DRF isn't a package 
that will be installed by default on most existing installs, I was 
trying to prevent breakage for them.

>> +    import patchwork.views.rest_api
>> +    urlpatterns += patchwork.views.rest_api.urlpatterns
>> +
>>   # redirect from old urls
>>   if settings.COMPAT_REDIR:
>>       urlpatterns += [


>> diff --git a/requirements-test.txt b/requirements-test.txt
>> index 2d0b32c..fb6a8c8 100644
>> --- a/requirements-test.txt
>> +++ b/requirements-test.txt
>> @@ -2,3 +2,4 @@ mysqlclient==1.3.7  # replace this with psycopg2 for a PostgreSQL backend
>>   django-debug-toolbar==1.4
>>   python-dateutil>2.0,<3.0
>>   selenium>2.0,<3.0
>> +djangorestframework==3.3.3
>
> This can be a pain to keep current. How about we trust that DRF are
> doing their versioning correctly and limit to >=3.3,<3.4? I think
> I changed this for the other dependendencies recently enough.

good idea.