Repository: falconry/falcon
Pull request #1286 (2018-05-14 18:21:36)

refactor(error_handler): Reorder Custom ErrorHandler params
This is a breaking Change.
Closes: https://github.com/falconry/falcon/issues/763

steven-upside: @kgriffs It would be possible to support old parameter order and new parameter order simultaneously by calling using the keyword style:

```
        for err_type, err_handler in self._error_handlers:
            if isinstance(ex, err_type):
                try:
                    err_handler(req=req, resp=resp, ex=ex, params=params)
                <...>
```
however error handlers would then have to use those exact names for their parameters. Users who don't currently do that would still experience a breaking change, so I'm not sure if its better or worse compatibility-wise, and it feels very different from other methods where order matters to have exactly one function which cares about names instead.

I think forced re-ordering is the preferable breaking change
codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1286?src=pr&el=h1) Report
> Merging [#1286](https://codecov.io/gh/falconry/falcon/pull/1286?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/d9efe4f6d9e5a59f4fcacebfe867749f19004191?src=pr&el=desc) will **decrease** coverage by `1.9%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master    #1286      +/-   ##
==========================================
- Coverage     100%   98.09%   -1.91%     
==========================================
  Files          39       39              
  Lines        2514     2514              
  Branches      378      363      -15     
==========================================
- Hits         2514     2466      -48     
- Misses          0       40      +40     
- Partials        0        8       +8
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1286?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/media/validators/jsonschema.py](https://codecov.io/gh/falconry/falcon/pull/1286/diff?src=pr&el=tree#diff-ZmFsY29uL21lZGlhL3ZhbGlkYXRvcnMvanNvbnNjaGVtYS5weQ==) | `47.82% <0%> (-52.18%)` | :arrow_down: |
| [falcon/util/uri.py](https://codecov.io/gh/falconry/falcon/pull/1286/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvdXJpLnB5) | `86.04% <0%> (-13.96%)` | :arrow_down: |
| [falcon/response\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1286/diff?src=pr&el=tree#diff-ZmFsY29uL3Jlc3BvbnNlX2hlbHBlcnMucHk=) | `92.15% <0%> (-7.85%)` | :arrow_down: |
| [falcon/util/misc.py](https://codecov.io/gh/falconry/falcon/pull/1286/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvbWlzYy5weQ==) | `96.93% <0%> (-3.07%)` | :arrow_down: |
| [falcon/testing/helpers.py](https://codecov.io/gh/falconry/falcon/pull/1286/diff?src=pr&el=tree#diff-ZmFsY29uL3Rlc3RpbmcvaGVscGVycy5weQ==) | `97.4% <0%> (-2.6%)` | :arrow_down: |
| [falcon/request.py](https://codecov.io/gh/falconry/falcon/pull/1286/diff?src=pr&el=tree#diff-ZmFsY29uL3JlcXVlc3QucHk=) | `98.6% <0%> (-1.4%)` | :arrow_down: |
| [falcon/response.py](https://codecov.io/gh/falconry/falcon/pull/1286/diff?src=pr&el=tree#diff-ZmFsY29uL3Jlc3BvbnNlLnB5) | `98.7% <0%> (-1.3%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1286?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/1286?src=pr&el=footer). Last update [d9efe4f...7368589](https://codecov.io/gh/falconry/falcon/pull/1286?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/falcon/api.py b/falcon/api.py
index 65e0d76..bfb1a02 100644
--- a/falcon/api.py
+++ b/falcon/api.py
@@ -228,7 +228,7 @@ class API(object):
                 # HTTP exception signalling the problem, e.g. a 404.
                 responder, params, resource, req.uri_template = self._get_responder(req)
             except Exception as ex:
-                if not self._handle_exception(ex, req, resp, params):
+                if not self._handle_exception(req, resp, ex, params):
                     raise
             else:
                 try:
@@ -244,7 +244,7 @@ class API(object):
                     responder(req, resp, **params)
                     req_succeeded = True
                 except Exception as ex:
-                    if not self._handle_exception(ex, req, resp, params):
+                    if not self._handle_exception(req, resp, ex, params):
                         raise
         finally:
             # NOTE(kgriffs): It may not be useful to still execute
@@ -259,7 +259,7 @@ class API(object):
                 try:
                     process_response(req, resp, resource, req_succeeded)
                 except Exception as ex:
-                    if not self._handle_exception(ex, req, resp, params):
+                    if not self._handle_exception(req, resp, ex, params):
                         raise
 
                     req_succeeded = False
@@ -492,7 +492,7 @@ class API(object):
                 that is an instance of this exception class, the associated
                 handler will be called.
             handler (callable): A function or callable object taking the form
-                ``func(ex, req, resp, params)``.
+                ``func(req, resp, ex, params)``.
 
                 If not specified explicitly, the handler will default to
                 ``exception.handle``, where ``exception`` is the error
@@ -503,7 +503,7 @@ class API(object):
                     class CustomException(CustomBaseException):
 
                         @staticmethod
-                        def handle(ex, req, resp, params):
+                        def handle(req, resp, ex, params):
                             # TODO: Log the error
                             # Convert to an instance of falcon.HTTPError
                             raise falcon.HTTPError(falcon.HTTP_792)
@@ -673,13 +673,13 @@ class API(object):
         if error.has_representation:
             self._serialize_error(req, resp, error)
 
-    def _http_status_handler(self, status, req, resp, params):
+    def _http_status_handler(self, req, resp, status, params):
         self._compose_status_response(req, resp, status)
 
-    def _http_error_handler(self, error, req, resp, params):
+    def _http_error_handler(self, req, resp, error, params):
         self._compose_error_response(req, resp, error)
 
-    def _handle_exception(self, ex, req, resp, params):
+    def _handle_exception(self, req, resp, ex, params):
         """Handle an exception raised from mw or a responder.
 
         Args:
@@ -699,7 +699,7 @@ class API(object):
         for err_type, err_handler in self._error_handlers:
             if isinstance(ex, err_type):
                 try:
-                    err_handler(ex, req, resp, params)
+                    err_handler(req, resp, ex, params)
                 except HTTPStatus as status:
                     self._compose_status_response(req, resp, status)
                 except HTTPError as error:


Test patch:
diff --git a/tests/test_error_handlers.py b/tests/test_error_handlers.py
index 40f3bbd..af478af 100644
--- a/tests/test_error_handlers.py
+++ b/tests/test_error_handlers.py
@@ -4,12 +4,12 @@ import falcon
 from falcon import testing
 
 
-def capture_error(ex, req, resp, params):
+def capture_error(req, resp, ex, params):
     resp.status = falcon.HTTP_723
     resp.body = 'error: %s' % str(ex)
 
 
-def handle_error_first(ex, req, resp, params):
+def handle_error_first(req, resp, ex, params):
     resp.status = falcon.HTTP_200
     resp.body = 'first error handler'
 
@@ -21,7 +21,7 @@ class CustomBaseException(Exception):
 class CustomException(CustomBaseException):
 
     @staticmethod
-    def handle(ex, req, resp, params):
+    def handle(req, resp, ex, params):
         raise falcon.HTTPError(
             falcon.HTTP_792,
             u'Internet crashed!',
diff --git a/tests/test_example.py b/tests/test_example.py
index 69d462f..d9b6c8e 100644
--- a/tests/test_example.py
+++ b/tests/test_example.py
@@ -24,7 +24,7 @@ class StorageEngine(object):
 class StorageError(Exception):
 
     @staticmethod
-    def handle(ex, req, resp, params):
+    def handle(req, resp, ex, params):
         description = ('Sorry, couldn\'t write your thing to the '
                        'database. It worked on my box.')
 
diff --git a/tests/test_middleware.py b/tests/test_middleware.py
index b9bc8ce..31c206b 100644
--- a/tests/test_middleware.py
+++ b/tests/test_middleware.py
@@ -351,7 +351,7 @@ class TestSeveralMiddlewares(TestMiddleware):
                                      RequestTimeMiddleware(),
                                      RaiseErrorMiddleware()])
 
-        def handler(ex, req, resp, params):
+        def handler(req, resp, ex, params):
             context['error_handler'] = True
 
         app.add_error_handler(Exception, handler)
@@ -381,7 +381,7 @@ class TestSeveralMiddlewares(TestMiddleware):
                                      RaiseErrorMiddleware(),
                                      RequestTimeMiddleware()])
 
-        def handler(ex, req, resp, params):
+        def handler(req, resp, ex, params):
             context['error_handler'] = True
 
         app.add_error_handler(Exception, handler)
@@ -411,7 +411,7 @@ class TestSeveralMiddlewares(TestMiddleware):
                                      RaiseErrorMiddleware(),
                                      ExecutedLastMiddleware()])
 
-        def handler(ex, req, resp, params):
+        def handler(req, resp, ex, params):
             pass
 
         app.add_error_handler(Exception, handler)
@@ -446,7 +446,7 @@ class TestSeveralMiddlewares(TestMiddleware):
                                      RaiseErrorMiddleware(),
                                      ExecutedLastMiddleware()])
 
-        def handler(ex, req, resp, params):
+        def handler(req, resp, ex, params):
             pass
 
         app.add_error_handler(Exception, handler)
@@ -480,7 +480,7 @@ class TestSeveralMiddlewares(TestMiddleware):
                                      RaiseErrorMiddleware(),
                                      ExecutedLastMiddleware()])
 
-        def handler(ex, req, resp, params):
+        def handler(req, resp, ex, params):
             pass
 
         app.add_error_handler(Exception, handler)
@@ -511,7 +511,7 @@ class TestSeveralMiddlewares(TestMiddleware):
                                      RaiseErrorMiddleware(),
                                      ExecutedLastMiddleware()])
 
-        def handler(ex, req, resp, params):
+        def handler(req, resp, ex, params):
             pass
 
         app.add_error_handler(Exception, handler)
@@ -542,7 +542,7 @@ class TestSeveralMiddlewares(TestMiddleware):
                                      RaiseErrorMiddleware(),
                                      ExecutedLastMiddleware()])
 
-        def handler(ex, req, resp, params):
+        def handler(req, resp, ex, params):
             pass
 
         app.add_error_handler(Exception, handler)
@@ -576,7 +576,7 @@ class TestSeveralMiddlewares(TestMiddleware):
                                      RaiseErrorMiddleware(),
                                      ExecutedLastMiddleware()])
 
-        def handler(ex, req, resp, params):
+        def handler(req, resp, ex, params):
             pass
 
         app.add_error_handler(Exception, handler)