From 7095021db7a3e2a905979c17f5af8fce614edeb9 Mon Sep 17 00:00:00 2001 From: Ryan P Kilby Date: Fri, 6 Jul 2018 05:03:12 -0400 Subject: [PATCH] Rename base_name => basename for consistency's sake (#5990) * Rename base_name => basename for consistency * Update tests to use basename --- docs/api-guide/routers.md | 16 ++++---- docs/api-guide/viewsets.md | 4 +- docs/topics/release-notes.md | 13 ++++++ rest_framework/routers.py | 40 +++++++++++++----- tests/test_routers.py | 79 +++++++++++++++++++++++++++++++++--- tests/test_schemas.py | 10 ++--- tests/test_viewsets.py | 6 +-- 7 files changed, 134 insertions(+), 34 deletions(-) diff --git a/docs/api-guide/routers.md b/docs/api-guide/routers.md index c39cda3ba..d3ebd6866 100644 --- a/docs/api-guide/routers.md +++ b/docs/api-guide/routers.md @@ -28,7 +28,7 @@ There are two mandatory arguments to the `register()` method: Optionally, you may also specify an additional argument: -* `base_name` - The base to use for the URL names that are created. If unset the basename will be automatically generated based on the `queryset` attribute of the viewset, if it has one. Note that if the viewset does not include a `queryset` attribute then you must set `base_name` when registering the viewset. +* `basename` - The base to use for the URL names that are created. If unset the basename will be automatically generated based on the `queryset` attribute of the viewset, if it has one. Note that if the viewset does not include a `queryset` attribute then you must set `basename` when registering the viewset. The example above would generate the following URL patterns: @@ -39,13 +39,13 @@ The example above would generate the following URL patterns: --- -**Note**: The `base_name` argument is used to specify the initial part of the view name pattern. In the example above, that's the `user` or `account` part. +**Note**: The `basename` argument is used to specify the initial part of the view name pattern. In the example above, that's the `user` or `account` part. -Typically you won't *need* to specify the `base_name` argument, but if you have a viewset where you've defined a custom `get_queryset` method, then the viewset may not have a `.queryset` attribute set. If you try to register that viewset you'll see an error like this: +Typically you won't *need* to specify the `basename` argument, but if you have a viewset where you've defined a custom `get_queryset` method, then the viewset may not have a `.queryset` attribute set. If you try to register that viewset you'll see an error like this: - 'base_name' argument not specified, and could not automatically determine the name from the viewset, as it does not have a '.queryset' attribute. + 'basename' argument not specified, and could not automatically determine the name from the viewset, as it does not have a '.queryset' attribute. -This means you'll need to explicitly set the `base_name` argument when registering the viewset, as it could not be automatically determined from the model name. +This means you'll need to explicitly set the `basename` argument when registering the viewset, as it could not be automatically determined from the model name. --- @@ -53,7 +53,7 @@ This means you'll need to explicitly set the `base_name` argument when registeri The `.urls` attribute on a router instance is simply a standard list of URL patterns. There are a number of different styles for how you can include these URLs. -For example, you can append `router.urls` to a list of existing views… +For example, you can append `router.urls` to a list of existing views... router = routers.SimpleRouter() router.register(r'users', UserViewSet) @@ -65,7 +65,7 @@ For example, you can append `router.urls` to a list of existing views… urlpatterns += router.urls -Alternatively you can use Django's `include` function, like so… +Alternatively you can use Django's `include` function, like so... urlpatterns = [ url(r'^forgot-password/$', ForgotPasswordFormView.as_view()), @@ -302,7 +302,7 @@ For another example of setting the `.routes` attribute, see the source code for If you want to provide totally custom behavior, you can override `BaseRouter` and override the `get_urls(self)` method. The method should inspect the registered viewsets and return a list of URL patterns. The registered prefix, viewset and basename tuples may be inspected by accessing the `self.registry` attribute. -You may also want to override the `get_default_base_name(self, viewset)` method, or else always explicitly set the `base_name` argument when registering your viewsets with the router. +You may also want to override the `get_default_basename(self, viewset)` method, or else always explicitly set the `basename` argument when registering your viewsets with the router. # Third Party Packages diff --git a/docs/api-guide/viewsets.md b/docs/api-guide/viewsets.md index 9be62bf16..d98f518b3 100644 --- a/docs/api-guide/viewsets.md +++ b/docs/api-guide/viewsets.md @@ -51,7 +51,7 @@ Typically we wouldn't do this, but would instead register the viewset with a rou from rest_framework.routers import DefaultRouter router = DefaultRouter() - router.register(r'users', UserViewSet, base_name='user') + router.register(r'users', UserViewSet, basename='user') urlpatterns = router.urls Rather than writing your own viewsets, you'll often want to use the existing base classes that provide a default set of behavior. For example: @@ -269,7 +269,7 @@ Note that you can use any of the standard attributes or method overrides provide def get_queryset(self): return self.request.user.accounts.all() -Note however that upon removal of the `queryset` property from your `ViewSet`, any associated [router][routers] will be unable to derive the base_name of your Model automatically, and so you will have to specify the `base_name` kwarg as part of your [router registration][routers]. +Note however that upon removal of the `queryset` property from your `ViewSet`, any associated [router][routers] will be unable to derive the basename of your Model automatically, and so you will have to specify the `basename` kwarg as part of your [router registration][routers]. Also note that although this class provides the complete set of create/list/retrieve/update/destroy actions by default, you can restrict the available operations by using the standard permission classes. diff --git a/docs/topics/release-notes.md b/docs/topics/release-notes.md index 2acf55762..e9f8d5abc 100644 --- a/docs/topics/release-notes.md +++ b/docs/topics/release-notes.md @@ -38,6 +38,16 @@ You can determine your currently installed version using `pip show`: --- +## 3.9.x series + +### 3.9.0 + +**Date**: Unreleased + +* Deprecate the `Router.register` `base_name` argument in favor of `basename`. [#5990][gh5990] +* Deprecate the `Router.get_default_base_name` method in favor of `Router.get_default_basename`. [#5990][gh5990] + + ## 3.8.x series ### 3.8.2 @@ -1961,3 +1971,6 @@ For older release notes, [please see the version 2.x documentation][old-release- [gh5922]: https://github.com/encode/django-rest-framework/issues/5922 [gh5921]: https://github.com/encode/django-rest-framework/issues/5921 [gh5920]: https://github.com/encode/django-rest-framework/issues/5920 + + +[gh5990]: https://github.com/encode/django-rest-framework/issues/5990 diff --git a/rest_framework/routers.py b/rest_framework/routers.py index 52b2b7cc6..392d43f79 100644 --- a/rest_framework/routers.py +++ b/rest_framework/routers.py @@ -22,6 +22,8 @@ from collections import OrderedDict, namedtuple from django.conf.urls import url from django.core.exceptions import ImproperlyConfigured from django.urls import NoReverseMatch +from django.utils import six +from django.utils.deprecation import RenameMethodsBase from rest_framework import views from rest_framework.response import Response @@ -73,21 +75,37 @@ def flatten(list_of_lists): return itertools.chain(*list_of_lists) -class BaseRouter(object): +class RenameRouterMethods(RenameMethodsBase): + renamed_methods = ( + ('get_default_base_name', 'get_default_basename', DeprecationWarning), + ) + + +class BaseRouter(six.with_metaclass(RenameRouterMethods)): def __init__(self): self.registry = [] - def register(self, prefix, viewset, base_name=None): - if base_name is None: - base_name = self.get_default_base_name(viewset) - self.registry.append((prefix, viewset, base_name)) + def register(self, prefix, viewset, basename=None, base_name=None): + if base_name is not None: + msg = "The `base_name` argument has been deprecated in favor of `basename`." + warnings.warn(msg, DeprecationWarning, 2) - def get_default_base_name(self, viewset): + assert not (basename and base_name), ( + "Do not provide both the `basename` and `base_name` arguments.") + + if basename is None: + basename = base_name + + if basename is None: + basename = self.get_default_basename(viewset) + self.registry.append((prefix, viewset, basename)) + + def get_default_basename(self, viewset): """ - If `base_name` is not specified, attempt to automatically determine + If `basename` is not specified, attempt to automatically determine it from the viewset. """ - raise NotImplementedError('get_default_base_name must be overridden') + raise NotImplementedError('get_default_basename must be overridden') def get_urls(self): """ @@ -151,14 +169,14 @@ class SimpleRouter(BaseRouter): self.trailing_slash = '/' if trailing_slash else '' super(SimpleRouter, self).__init__() - def get_default_base_name(self, viewset): + def get_default_basename(self, viewset): """ - If `base_name` is not specified, attempt to automatically determine + If `basename` is not specified, attempt to automatically determine it from the viewset. """ queryset = getattr(viewset, 'queryset', None) - assert queryset is not None, '`base_name` argument not specified, and could ' \ + assert queryset is not None, '`basename` argument not specified, and could ' \ 'not automatically determine the name from the viewset, as ' \ 'it does not have a `.queryset` attribute.' diff --git a/tests/test_routers.py b/tests/test_routers.py index 8f52d217f..1dd2d2b0a 100644 --- a/tests/test_routers.py +++ b/tests/test_routers.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals +import warnings from collections import namedtuple import pytest @@ -86,13 +87,13 @@ kwarged_notes_router = SimpleRouter() kwarged_notes_router.register(r'notes', KWargedNoteViewSet) namespaced_router = DefaultRouter() -namespaced_router.register(r'example', MockViewSet, base_name='example') +namespaced_router.register(r'example', MockViewSet, basename='example') empty_prefix_router = SimpleRouter() -empty_prefix_router.register(r'', EmptyPrefixViewSet, base_name='empty_prefix') +empty_prefix_router.register(r'', EmptyPrefixViewSet, basename='empty_prefix') regex_url_path_router = SimpleRouter() -regex_url_path_router.register(r'', RegexUrlPathViewSet, base_name='regex') +regex_url_path_router.register(r'', RegexUrlPathViewSet, basename='regex') class BasicViewSet(viewsets.ViewSet): @@ -319,7 +320,7 @@ class TestActionKeywordArgs(TestCase): }) self.router = SimpleRouter() - self.router.register(r'test', TestViewSet, base_name='test') + self.router.register(r'test', TestViewSet, basename='test') self.view = self.router.urls[-1].callback def test_action_kwargs(self): @@ -344,7 +345,7 @@ class TestActionAppliedToExistingRoute(TestCase): }) self.router = SimpleRouter() - self.router.register(r'test', TestViewSet, base_name='test') + self.router.register(r'test', TestViewSet, basename='test') with pytest.raises(ImproperlyConfigured): self.router.urls @@ -481,3 +482,71 @@ class TestViewInitkwargs(URLPatternsTestCase, TestCase): initkwargs = match.func.initkwargs assert initkwargs['basename'] == 'routertestmodel' + + +class TestBaseNameRename(TestCase): + + def test_base_name_and_basename_assertion(self): + router = SimpleRouter() + + msg = "Do not provide both the `basename` and `base_name` arguments." + with warnings.catch_warnings(record=True) as w, \ + self.assertRaisesMessage(AssertionError, msg): + warnings.simplefilter('always') + router.register('mock', MockViewSet, 'mock', base_name='mock') + + msg = "The `base_name` argument has been deprecated in favor of `basename`." + assert len(w) == 1 + assert str(w[0].message) == msg + + def test_base_name_argument_deprecation(self): + router = SimpleRouter() + + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter('always') + router.register('mock', MockViewSet, base_name='mock') + + msg = "The `base_name` argument has been deprecated in favor of `basename`." + assert len(w) == 1 + assert str(w[0].message) == msg + assert router.registry == [ + ('mock', MockViewSet, 'mock'), + ] + + def test_basename_argument_no_warnings(self): + router = SimpleRouter() + + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter('always') + router.register('mock', MockViewSet, basename='mock') + + assert len(w) == 0 + assert router.registry == [ + ('mock', MockViewSet, 'mock'), + ] + + def test_get_default_base_name_deprecation(self): + msg = "`CustomRouter.get_default_base_name` method should be renamed `get_default_basename`." + + # Class definition should raise a warning + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter('always') + + class CustomRouter(SimpleRouter): + def get_default_base_name(self, viewset): + return 'foo' + + assert len(w) == 1 + assert str(w[0].message) == msg + + # Deprecated method implementation should still be called + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter('always') + + router = CustomRouter() + router.register('mock', MockViewSet) + + assert len(w) == 0 + assert router.registry == [ + ('mock', MockViewSet, 'foo'), + ] diff --git a/tests/test_schemas.py b/tests/test_schemas.py index c2a429ac3..352061aed 100644 --- a/tests/test_schemas.py +++ b/tests/test_schemas.py @@ -122,7 +122,7 @@ else: pass router = DefaultRouter() -router.register('example', ExampleViewSet, base_name='example') +router.register('example', ExampleViewSet, basename='example') urlpatterns = [ url(r'^$', schema_view), url(r'^', include(router.urls)) @@ -509,7 +509,7 @@ class TestSchemaGeneratorNotAtRoot(TestCase): class TestSchemaGeneratorWithMethodLimitedViewSets(TestCase): def setUp(self): router = DefaultRouter() - router.register('example1', MethodLimitedViewSet, base_name='example1') + router.register('example1', MethodLimitedViewSet, basename='example1') self.patterns = [ url(r'^', include(router.urls)) ] @@ -566,8 +566,8 @@ class TestSchemaGeneratorWithMethodLimitedViewSets(TestCase): class TestSchemaGeneratorWithRestrictedViewSets(TestCase): def setUp(self): router = DefaultRouter() - router.register('example1', Http404ExampleViewSet, base_name='example1') - router.register('example2', PermissionDeniedExampleViewSet, base_name='example2') + router.register('example1', Http404ExampleViewSet, basename='example1') + router.register('example2', PermissionDeniedExampleViewSet, basename='example2') self.patterns = [ url('^example/?$', ExampleListView.as_view()), url(r'^', include(router.urls)) @@ -1042,7 +1042,7 @@ class NamingCollisionViewSet(GenericViewSet): naming_collisions_router = SimpleRouter() -naming_collisions_router.register(r'collision', NamingCollisionViewSet, base_name="collision") +naming_collisions_router.register(r'collision', NamingCollisionViewSet, basename="collision") @pytest.mark.skipif(not coreapi, reason='coreapi is not installed') diff --git a/tests/test_viewsets.py b/tests/test_viewsets.py index f3e85050e..54b691e22 100644 --- a/tests/test_viewsets.py +++ b/tests/test_viewsets.py @@ -65,7 +65,7 @@ class ActionViewSet(GenericViewSet): router = SimpleRouter() router.register(r'actions', ActionViewSet) -router.register(r'actions-alt', ActionViewSet, base_name='actions-alt') +router.register(r'actions-alt', ActionViewSet, basename='actions-alt') urlpatterns = [ @@ -177,7 +177,7 @@ class GetExtraActionUrlMapTests(TestCase): class ReverseActionTests(TestCase): def test_default_basename(self): view = ActionViewSet() - view.basename = router.get_default_base_name(ActionViewSet) + view.basename = router.get_default_basename(ActionViewSet) view.request = None assert view.reverse_action('list') == '/api/actions/' @@ -203,7 +203,7 @@ class ReverseActionTests(TestCase): def test_request_passing(self): view = ActionViewSet() - view.basename = router.get_default_base_name(ActionViewSet) + view.basename = router.get_default_basename(ActionViewSet) view.request = factory.get('/') # Passing the view's request object should result in an absolute URL.