#1121 Deprecate debug parameter of make_handler()
#1123
Conversation
|
@asvetlov Please check if this reflects your vision. Tests are coming soon. |
f0t0n
force-pushed
the
feature/app-make-handler-debug-param
branch
2 times, most recently
from
6a20fcf to
172be9a
Compare
Current coverage is 98.17% (diff: 100%)@@ master #1123 diff @@
==========================================
Files 28 28
Lines 6392 6398 +6
Methods 0 0
Messages 0 0
Branches 1076 1078 +2
==========================================
+ Hits 6275 6281 +6
Misses 63 63
Partials 54 54
|
f0t0n
force-pushed
the
feature/app-make-handler-debug-param
branch
from
172be9a to
c20dd31
Compare
|
Added test coverage. Ready for review. |
f0t0n
force-pushed
the
feature/app-make-handler-debug-param
branch
3 times, most recently
from
6017d8a to
1add498
Compare
| @@ -221,6 +221,22 @@ def middlewares(self): | |||
| return self._middlewares | |||
|
|
|||
| def make_handler(self, **kwargs): | |||
| try: | |||
| debug = kwargs['debug'] | |||
There was a problem hiding this comment.
I prefer debug = kwargs.pop('debug', _sentinel)
Please use this form, it's a little shorter and faster than exception based approach.
There was a problem hiding this comment.
In the issue description you said:
If app.debug conflicts with explicitly passed debug param -- raise ValueError.
So that if we put default boolean value to kwargs.pop() we can't know if the key was in the dictionary or not.
Also we want to compare this value to self.debug and raise a ValueError if they differ but only if the value was there.
Of course we can use None as a default value and then check for None.
debug = kwargs.pop('debug', None)
if debug is not None and debug != self.debug:
raise ValueError("...")
return self._handler_factory(self, self.router, debug=self.debug, loop=self.loop, **kwargs)
There was a problem hiding this comment.
helpers._sentinel is a marker which could be used even if None is proper value.
Not necessary for boolean parameters though.
There was a problem hiding this comment.
Ah, got you. Didn't see that object before. Thanks.
There was a problem hiding this comment.
The changes you've suggested are done.
f0t0n
force-pushed
the
feature/app-make-handler-debug-param
branch
2 times, most recently
from
5480568 to
02d60c9
Compare
| @@ -1128,6 +1135,13 @@ duplicated like one using :meth:`Application.copy`. | |||
| :param kwargs: additional parameters for :class:`RequestHandlerFactory` | |||
| constructor. | |||
|
|
|||
| .. note:: | |||
There was a problem hiding this comment.
Use .. deprecated:: 1.0 directive.
There was a problem hiding this comment.
I thought to do this but the parameters of make_handler are not described and there're just **kwargs.
I can add debug to the method signature and debug parameter description and deprecation hint but I don't think it's good idea to add deprecated parameter to signature now (before the release which deprecates this).
If I just use .. deprecated:: 1.0 and the text to me it looks ugly and could be confusing.
What do you think?
There was a problem hiding this comment.
could be confusing
Namely user may ask "what's deprecated, kwargs or make_handler method? o_O".
There was a problem hiding this comment.
Just add identation to push deprecation directive under parameter spec.
There was a problem hiding this comment.
Ok, done.
I just doubt if this could confuse user and he could briefly read this as
kwargs - additional parameters for RequestHandlerFactory constructor.
deprecated since 1.0: (blah-blah tl;dr!)
and he thinks "ok probably those kwargs are deprecated".
There was a problem hiding this comment.
The best solution is explicitly document all available parameters.
It makes sense not only for getting rid of weird deprecation.
I suspect the most part of users just don't know what parameters are available.
But it's a subject for another PR.
Feel free to create it if you want.
f0t0n
force-pushed
the
feature/app-make-handler-debug-param
branch
from
02d60c9 to
40741d2
Compare
- Deprecate `debug` parameter of `Application.make_handler`
- Add docs for `debug` parameter in the `Application`'s constructor
- Fix docs display for `Application`'s `loop` parameter
- Add docs for `Application.debug` attribute
- Add a note about the deprecation of the `debug` parameter for
`Application.make_handler()`
- Test make_handler debug deprecation
f0t0n
force-pushed
the
feature/app-make-handler-debug-param
branch
from
40741d2 to
bd5f47c
Compare
|
Thanks! |
What do these changes do?
debugparameter ofApplication.make_handlerdebugparameter in theApplication's constructorApplication'sloopparameterApplication.debugattributedebugparameter forApplication.make_handler()Are there changes in behavior for the user?
From now
Application.make_handlerwill use application's debug mode.If
debugparameter is passed to this method and differs with the value ofApplication.debug,the
ValueErrorwill be raised.Related issue number
#1121
Checklist