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

Preserve wsgify default args #203

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@nickstenning
Copy link
Contributor

nickstenning commented Jun 10, 2015

wsgify objects can be created with args/kwargs at construction time:

def hello(req, name):
    return "Hello, %s!" % name
app = wsgify(hello, args=("Fred",))

Previously, these arguments would only be passed to the underlying
function (here, hello) when using the wsgify object as a callable that
accepts (environ, start_response). If you used the object as a
callable taking a request (as documented) then the default args would be
discarded.

That is, continuing the above example:

req = Request.blank('/')
resp = req.get_response(app)  # => "Hello, Fred"

BUT

app(req)                      # => raises TypeError

This is perhaps most confusing when it interacts with
wsgify.middleware. For example:

@wsgify.middleware
def add_magic_header(req, app, value='seated ducks'):
    resp = req.get_response(app)
    resp.headers['Magic-Header'] = value
    return resp

app = add_magic_header(app, value='flying elephants')

resp1 = req.get_response(app)
resp2 = app(req)

resp1.headers['Magic-Header']  # => 'flying elephants'
resp2.headers['Magic-Header']  # => 'seated ducks'

(An executable example of the above can be found here.)

This pull request ensures that if variadic arguments or keyword arguments are
not provided in a call like

app(req)

then the defaults are used.

@nickstenning

This comment has been minimized.

Copy link
Contributor

nickstenning commented Jun 10, 2015

As discussed with @mmerickel in #pyramid.

@nickstenning

This comment has been minimized.

Copy link
Contributor

nickstenning commented Jun 10, 2015

I wrote up this patch last night, and I've been thinking about it some more.

I'm not sure this is the right thing to do.

Specifically, while the difference between req.get_response(app) and app(req) is confusing, the change in this patch:

  • is backwards-incompatible
  • makes it impossible to override the default arguments with no arguments

In addition, the whole story of default arguments seems a bit confusing:

  • wsgify takes explicit args and kw params
  • wsgify.middleware takes **kw only

So I'm submitting this for discussion, because it may be that the correct fix is actually some clearer documentation about when default arguments are used and when they are not. (i.e. "If you use the resulting callable as app(req), then you are deemed to be overriding the default arguments").

I'd be interested to hear what others think.

nickstenning added some commits Jun 9, 2015

Add some tests for wsgify/wsgify.middleware args
When treating the result of `wsgify` or `wsgify.middleware` as a
callable, arguments passed to the args/kwargs options to wsgify (and, by
extension, to the middleware factory) are currently being dropped.

This adds a couple of tests to demonstrate this, as well as a couple of
tests to ensure that overriding args from the actual callable invocation
remains possible.

N.B. middleware only supports passing kwargs (and not args) through from
the intermediary stages (i.e. from unbound middleware -> middleware
factory -> app).
Correctly preserve args when using wsgify callables
wsgify objects can be created with args/kwargs at construction time:

    def hello(req, name):
        return "Hello, %s!" % name
    app = wsgify(hello, args=("Fred",))

Previously, these arguments would only be passed to the underlying
function (here, `hello`) when using the wsgify object as a callable that
accepts `(environ, start_response)`. If you used the object as a
callable taking a request (as documented) then the default args would be
discarded.

That is, continuing the above example:

    req = Request.blank('/')
    resp = req.get_response(app)  # => "Hello, Fred"

    BUT

    app(req)                      # => raises TypeError

This is perhaps most confusing when it interacts with
`wsgify.middleware`. For example:

     @wsgify.middleware
     def add_magic_header(req, app, value='seated ducks'):
         resp = req.get_response(app)
         resp.headers['Magic-Header'] = value
         return resp

     app = add_magic_header(app, value='flying elephants')

     resp1 = req.get_response(app)
     resp2 = app(req)

     resp1.headers['Magic-Header']  # => 'flying elephants'
     resp2.headers['Magic-Header']  # => 'seated ducks'

This commit ensures that if variadic arguments or keyword arguments are
not provided in a call like

    app(req)

then the defaults are used.

@nickstenning nickstenning force-pushed the nickstenning:preserve-default-wsgify-args branch from 31c1595 to ab38688 Jun 10, 2015

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jul 30, 2016

Since your last comment, have you had any other thoughts on how you expect this behave? If not I was planning on closing this PR for now. I don't have a good answer to any of your questions.

@mmerickel

This comment has been minimized.

Copy link
Member

mmerickel commented Aug 1, 2016

I think it's surprising that the defaults are not used and so I think I'm in support of this PR.

@nickstenning nickstenning deleted the nickstenning:preserve-default-wsgify-args branch May 23, 2017

@nickstenning

This comment has been minimized.

Copy link
Contributor

nickstenning commented May 23, 2017

Heh. Thanks folks. 👍

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented May 23, 2017

We'll see what breaks when I do the next release. I think the fix is for the better! Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment