Repository: falconry/falcon
Pull request #1302 (2018-05-15 15:55:39)

refactor(routing): Require custom routers to accept extra options only via **kwargs
BREAKING CHANGE: 

Routers were previously allowed to accept additional args and
keyword arguments, and were not required to use the variadic form. Now,
they are only allowed to accept additional options as variadic keyword
arguments, and to ignore any arguments they don't support. This helps
overridden router logic be less fragile in terms of their interface
contracts, which also makes it easier to keep Falcon backwards-compatible
in the face of any future changes in this area.

BREAKING CHANGE: 

API.add_route() previously accepted *args, but no longer does.
Additionaly, suffix is now specified as an explicit keyword argument. This
means that if a single additional non-keyword argument was previously
passed, it will now erroneously be assigned to the suffix arg. This should
be a simple breaking change for apps to fix if they run into it.

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1302?src=pr&el=h1) Report
> Merging [#1302](https://codecov.io/gh/falconry/falcon/pull/1302?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/70fa31f9537f084f4947f8e1f163294f8abd099b?src=pr&el=desc) will **increase** coverage by `1.9%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1302/graphs/tree.svg?width=650&height=150&src=pr&token=zOtkfNQ3h6)](https://codecov.io/gh/falconry/falcon/pull/1302?src=pr&el=tree)

```diff
@@           Coverage Diff            @@
##           master   #1302     +/-   ##
========================================
+ Coverage   98.09%    100%   +1.9%     
========================================
  Files          38      38             
  Lines        2516    2515      -1     
  Branches      370     385     +15     
========================================
+ Hits         2468    2515     +47     
+ Misses         40       0     -40     
+ Partials        8       0      -8
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1302?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/routing/compiled.py](https://codecov.io/gh/falconry/falcon/pull/1302/diff?src=pr&el=tree#diff-ZmFsY29uL3JvdXRpbmcvY29tcGlsZWQucHk=) | `100% <100%> (ø)` | :arrow_up: |
| [falcon/response.py](https://codecov.io/gh/falconry/falcon/pull/1302/diff?src=pr&el=tree#diff-ZmFsY29uL3Jlc3BvbnNlLnB5) | `100% <0%> (+1.27%)` | :arrow_up: |
| [falcon/request.py](https://codecov.io/gh/falconry/falcon/pull/1302/diff?src=pr&el=tree#diff-ZmFsY29uL3JlcXVlc3QucHk=) | `100% <0%> (+1.33%)` | :arrow_up: |
| [falcon/testing/helpers.py](https://codecov.io/gh/falconry/falcon/pull/1302/diff?src=pr&el=tree#diff-ZmFsY29uL3Rlc3RpbmcvaGVscGVycy5weQ==) | `100% <0%> (+2.59%)` | :arrow_up: |
| [falcon/util/misc.py](https://codecov.io/gh/falconry/falcon/pull/1302/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvbWlzYy5weQ==) | `100% <0%> (+3.06%)` | :arrow_up: |
| [falcon/response\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1302/diff?src=pr&el=tree#diff-ZmFsY29uL3Jlc3BvbnNlX2hlbHBlcnMucHk=) | `100% <0%> (+7.27%)` | :arrow_up: |
| [falcon/util/uri.py](https://codecov.io/gh/falconry/falcon/pull/1302/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvdXJpLnB5) | `100% <0%> (+13.95%)` | :arrow_up: |
| [falcon/media/validators/jsonschema.py](https://codecov.io/gh/falconry/falcon/pull/1302/diff?src=pr&el=tree#diff-ZmFsY29uL21lZGlhL3ZhbGlkYXRvcnMvanNvbnNjaGVtYS5weQ==) | `100% <0%> (+52.17%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1302?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1302?src=pr&el=footer). Last update [70fa31f...da713fb](https://codecov.io/gh/falconry/falcon/pull/1302?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/docs/api/routing.rst b/docs/api/routing.rst
index 88453c0..d13cd39 100644
--- a/docs/api/routing.rst
+++ b/docs/api/routing.rst
@@ -239,7 +239,7 @@ A custom router is any class that implements the following interface:
 .. code:: python
 
     class MyRouter(object):
-        def add_route(self, uri_template, method_map, resource):
+        def add_route(self, uri_template, method_map, resource, **kwargs):
             """Adds a route between URI path template and resource.
 
             Args:
@@ -248,6 +248,14 @@ A custom router is any class that implements the following interface:
                     falcon.routing.create_http_method_map.
                 resource (object): Instance of the resource class that
                     will handle requests for the given URI.
+
+            Keyword Arguments:
+
+                **kwargs (dict): Accepts any additional keyword arguments that
+                    were originally passed to the falcon.API.add_route() method.
+                    These arguments MUST be accepted via the double-star
+                    variadic pattern (**kwargs), and ignore any unrecognized
+                    or unsupported arguments.
             """
 
         def find(self, uri, req=None):
diff --git a/falcon/api.py b/falcon/api.py
index f738a17..3979748 100644
--- a/falcon/api.py
+++ b/falcon/api.py
@@ -301,7 +301,7 @@ class API(object):
     def router_options(self):
         return self._router.options
 
-    def add_route(self, uri_template, resource, *args, **kwargs):
+    def add_route(self, uri_template, resource, suffix=None, **kwargs):
         """Associate a templatized URI path with a resource.
 
         Falcon routes incoming requests to resources based on a set of
@@ -342,12 +342,13 @@ class API(object):
                 resource.
 
         Note:
-            Any additional args and kwargs not defined above are passed
+            Any additional keyword arguments not defined above are passed
             through to the underlying router's ``add_route()`` method. The
-            default router does not expect any additional arguments, but
+            default router ignores any additional keyword arguments, but
             custom routers may take advantage of this feature to receive
-            additional options when setting up routes.
-
+            additional options when setting up routes. Custom routers MUST
+            accept such arguments using the variadic pattern (``**kwargs``), and
+            ignore any keyword arguments that they don't support.
         """
 
         # NOTE(richardolsson): Doing the validation here means it doesn't have
@@ -361,17 +362,9 @@ class API(object):
         if '//' in uri_template:
             raise ValueError("uri_template may not contain '//'")
 
-        # NOTE(santeyio): This is a not very nice way to catch the suffix
-        # keyword. In python 3 it can be specified explicitly in the function
-        # definition, e.g.
-        # `add_route(self, uri_template, resource, *args, suffix=None, **kwargs)`
-        # but python 2 won't accept args like this.
-        suffix = kwargs.pop('suffix', None)
-
         method_map = routing.map_http_methods(resource, suffix=suffix)
         routing.set_default_responders(method_map)
-        self._router.add_route(uri_template, method_map, resource, *args,
-                               **kwargs)
+        self._router.add_route(uri_template, method_map, resource, **kwargs)
 
     def add_static_route(self, prefix, directory, downloadable=False, fallback_filename=None):
         """Add a route to a directory of static files.
diff --git a/falcon/routing/compiled.py b/falcon/routing/compiled.py
index 22f6e22..4ac6622 100644
--- a/falcon/routing/compiled.py
+++ b/falcon/routing/compiled.py
@@ -89,7 +89,7 @@ class CompiledRouter(object):
     def finder_src(self):
         return self._finder_src
 
-    def add_route(self, uri_template, method_map, resource):
+    def add_route(self, uri_template, method_map, resource, **kwargs):
         """Adds a route between a URI path template and a resource.
 
         Args:


Test patch:
diff --git a/tests/test_custom_router.py b/tests/test_custom_router.py
index d453e42..a0ddea7 100644
--- a/tests/test_custom_router.py
+++ b/tests/test_custom_router.py
@@ -1,3 +1,5 @@
+import pytest
+
 import falcon
 from falcon import testing
 
@@ -70,7 +72,8 @@ def test_can_pass_additional_params_to_add_route():
     check = []
 
     class CustomRouter(object):
-        def add_route(self, uri_template, method_map, resource, name):
+        def add_route(self, uri_template, method_map, resource, **kwargs):
+            name = kwargs['name']
             self._index = {name: uri_template}
             check.append(name)
 
@@ -83,11 +86,11 @@ def test_can_pass_additional_params_to_add_route():
     assert len(check) == 1
     assert 'my-url-name' in check
 
-    # Also as arg.
-    app.add_route('/test', 'resource', 'my-url-name-arg')
-
-    assert len(check) == 2
-    assert 'my-url-name-arg' in check
+    # NOTE(kgriffs): Extra values must be passed as kwargs, since that makes
+    #   it a lot easier for overriden methods to simply ignore options they
+    #   don't care about.
+    with pytest.raises(TypeError):
+        app.add_route('/test', 'resource', 'xarg1', 'xarg2')
 
 
 def test_custom_router_takes_req_positional_argument():