Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use _MiddlewareFactory as decorator with arguments #228

Merged
merged 6 commits into from
Jul 30, 2016

Conversation

ryd994
Copy link

@ryd994 ryd994 commented Dec 29, 2015

This change allows _MiddlewareFactory to be used as decorator with argument:

@wsgify.middleware
def restrict_ip(req, app, ips):
    if req.remote_addr not in ips:
        raise webob.exc.HTTPForbidden('Bad IP: %s' % req.remote_addr)
    return app

@restrict_ip(ips=['127.0.0.1'])
@wsgify
def app(req):
      return 'hi'

@digitalresistor digitalresistor added this to the Version 1.6 milestone Dec 29, 2015
@mmerickel
Copy link
Member

@renyidong Could you update this PR with a test case showing your intended workflow? I think the idea itself is fine!

@digitalresistor digitalresistor removed this from the Version 1.6 milestone Jan 29, 2016
@ryd994
Copy link
Author

ryd994 commented Jan 29, 2016

@mmerickel Taking the example from http://docs.webob.org/en/latest/api/dec.html

@wsgify.middleware
def restrict_ip(req, app, ips):
    if req.remote_addr not in ips:
        raise webob.exc.HTTPForbidden('Bad IP: %s' % req.remote_addr)
    return app

@wsgify
def app(req):
    return 'hi'

wrapped = restrict_ip(app, ips=['127.0.0.1'])

I have to define an extra function and wrap it with the middleware.

With this patch, I would be able to achieve same effect as the example at very beginning.

@mmerickel
Copy link
Member

@renyidong I understand the patch. I was asking if you could please add a test case to the codebase to ensure this keeps working in the future.

@ryd994
Copy link
Author

ryd994 commented Jan 30, 2016

@mmerickel Apologies for my misinterpret. I appended test code to test_middleware(). Or you would prefer adding a separate test?

@digitalresistor
Copy link
Member

A separate test would be preferred. Thanks!

@ryd994
Copy link
Author

ryd994 commented Jan 31, 2016

@bertjwregeer Let me know if there is anything else you need.

@mmerickel
Copy link
Member

LGTM!

@digitalresistor digitalresistor added this to the 1.7.0 milestone Jul 30, 2016
@digitalresistor digitalresistor merged commit 686b859 into Pylons:master Jul 30, 2016
digitalresistor added a commit that referenced this pull request Jul 30, 2016
@digitalresistor
Copy link
Member

Sorry for taking so long! I've merged this into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants