bug 1401246: convert to new-style middleware #4841
Conversation
|
I think it would be more straightforward to use the |
|
No problem. I personally prefer the functional style (when there's no chance that the middleware might need to be sub-classed), but the class-style is great too. I'll update this with the class-style. |
escattone
changed the title
|
I don't have a strong preference for the class-style, I think you can pick and chose as appropriate. I was mostly thinking about a reduced diff, to figure out why tests are failing. |
|
I was surprised that the tests failed, as they worked for me locally last night. Sorry, I'll get that resolved. |
|
The tests were failing because I had inadvertently removed lines involving adding hostnames to |
|
FYI. The headless tests passed against my local dev ( |
kuma/core/middleware.py
Outdated
| translation.activate(language) | ||
| request.LANGUAGE_CODE = language | ||
| def __call__(self, request): | ||
| # Activate the language, based on the request. |
There was a problem hiding this comment.
You've made this comment redundant with the function name 🙂. However, it's not 100% clear from the name that request gains a LANGUAGE_CODE parameter, maybe that's worth including in the comment.
kuma/core/middleware.py
Outdated
|
|
||
| def __init__(self, get_response): | ||
| self.get_response = get_response | ||
| super(MiddlewareBase, self).__init__() |
There was a problem hiding this comment.
I think the super````__init__ is unneeded because the parent is object
There was a problem hiding this comment.
👍 I added it, thinking it would make it bullet-proof for the hypothetical case of this base class being used along with others in a multiple-inheritance case (to keep the chain of MRO calls going)...but I can't imagine a real case where that would be needed!
| class MiddlewareBase(object): | ||
|
|
||
| def __init__(self, get_response): | ||
| self.get_response = get_response |
There was a problem hiding this comment.
I like having this defined once.
kuma/core/middleware.py
Outdated
| """ | ||
| Restricts the accessible endpoints based on the host. | ||
| """ | ||
| def __call__(self, request): | ||
| if settings.ENABLE_RESTRICTIONS_BY_HOST and is_untrusted(request): |
There was a problem hiding this comment.
It may make sense to move this to the __init__:
def __init__(self, get_response):
if not settings.ENABLE_RESTRICTIONS_BY_HOST:
raise MiddlewareNotUsed
Or, I may be trying too hard to use every piece of the buffalo, and it would have a negative impact on tests...
There was a problem hiding this comment.
LOL ("every piece of the buffalo") Good point! I remember reading about that new feature, and this may just be the case! I like it because if it's not ever going to be used (settings.ENABLE_RESTRICTIONS_BY_HOST = False), it doesn't remain embedded in the middleware stack.
Codecov Report
@@ Coverage Diff @@
## master #4841 +/- ##
==========================================
+ Coverage 95.92% 95.92% +<.01%
==========================================
Files 273 273
Lines 24538 24549 +11
Branches 1763 1763
==========================================
+ Hits 23537 23548 +11
Misses 791 791
Partials 210 210
Continue to review full report at Codecov.
|
|
@jwhitlock Thanks for the review! I added some changes that you may want to review one more time. |
|
Changes look good, thanks @escattone! |
I did some work on our Kuma middleware a short while ago, but when I realized it needed Django 1.11 in place, I shelved it. I thought it was useful, as it mostly embraced the new,
functionalclass-style of writing middleware (exceptReadOnlyMiddleware), but it's real goal was to remove the dependency ondjango.utils.deprecation.MiddlewareMixinand go all-in with the new-style. I didn't want to lose the work, which wasn't totally finished, so I added what was missing and decided it was now or never.