Expose response class

[sontek]

This is a small clean-up to utilize the ResponseClass that is already available on the Request object. This allows developers to define their own request factory with a new response class and have it used everywhere.

This is an attempt to help with #1256

As was suggested in the ticket, it might make more sense to just create a set_response_factory API and a response_factory kwarg of the configurator, but since the ResponseClass is already there it doesn't hurt to utilize it. I can't think of a time you would want to create a response when you don't have a request which is why I opted to take this route instead.

[tseaver]

For clarity, _get_response_factory needs its own explicit tests (rather than testing it through callers).

[tseaver]

Other than that, LGTM.

[tseaver]

registry undefined, breaks Travis.

[sontek]

@tseaver Sorry about that, made a quick change and didn't test it locally. Its fixed now :)

[mmerickel]

This looks great. One small gripe. It looks like IResponseFactory overrides any default ResponseClass set on the request. This may not be what we want. I'd prefer (if possible) to define the lookup order as:

• request.response attribute
• request.ResponseClass
• request.registry.queryUtility(IResponseFactory)
• pyramid.response.Response

I'd opt for something like if request.ResponseClass is not Response: use whatever is set. Unfortunately this doesn't let you force it to be Response. SO what about turning Request.ResponseClass into a lazily computed property? The only problem there is that ResponseClass is always defined thanks to at least webob.Request.ResponseClass so I'm having trouble coming up with any magic to make that lookup-order work.

[mmerickel]

I've thought more about this and I retract my complaints. I was trying to make it such that if a user wanted to override the response factory they could do so via setting request.ResponseClass. However this is dumb. All they need to do is set request.response = MyCustomResponse() and they're good to go. The factory is only ever invoked if request.response is not already set!

I think the only thing left in this PR is to add a config.set_response_factory public API on top of the registry.

[sontek]

@mmerickel I've made the set_response_factory API. Someone should help me with the docs, I'm not very good at that type of thing but I took a quick stab at it based on some copy/paste from the request_factory docs.

[tseaver]

LGTM

[mmerickel]

Before we make this API public I want to raise one concern. I think the IResponseFactory should accept the request object. @tseaver can you respond?

[tseaver]

If we require the argument, some uses will drop it on the floor, but if we don't, then some uses might be left hanging. For instance, right now, our Response class (derived from the one in webob), doesn't take a request as an argument; the FileResponse does take one, optionally (so that it can get to the wsgi.filewrapper in the environ).

I think if we do make it required then we should override the default Response constructor to take (but ignore) it, so that the calling convention is uniform.

[mmerickel]

I think that sending the class as the factory is an anti-pattern and it'd be better to just config.set_response_factory(lambda r: Response()), no?

[sontek]

@mmerickel set_request_factory requires a class that implements the same interface as Request. I'm fine with changing this for set_response_factory but they will work differently at that point

[mmerickel]

We added request to __json__(self, request) on model objects. That's more pervasive and significant than adding request to the response factory and serves the same purpose of allowing the developer to access their settings. I'm just having a real difficult time coming up with a good reason not to require it for a public API. If we don't add it then the first person who wants it will be required to use the threadlocals. We don't want that.

[mmerickel]

If we are super concerned about breaking someone using the current private API then we could add another interface for the new public api but I don't think it's necessary.

[sontek]

@mmerickel Only thing I'm worried about at that point is the ResponseClass on the Request object. Would we just always ignore this class and only use the factory?

[sontek]

@mmerickel 32cb805 turns the response factory into a factory that takes a request and drops support for using ResponseClass as an entrypoint for configuring your response class

[digitalresistor]

I like the idea of being able to pass the request to the Response class, and even if we can't see why it is needed, someone will find some sort of use for it.

Could you also add an entry to CHANGES.txt?

[mmerickel]

@sontek yes I think we should ignore the fact that Request.ResponseClass and Response.RequestClass exist. They were inherited from webob and can disappear quietly imo.

[mmerickel]

This PR looks great. Final request is to please either a) remove _get_response_factory or b) move _get_response_factory into the pyramid.response module to avoid any future circular imports between pyramid.response and pyramid.util. I'll happily merge afterward!

[sontek]

@mmerickel I prefer having it because we can change the implementation in one place, I'll make the move though

[mmerickel]

Tank you SO MUCH

[marc1n]

@mmerickel This is backward incompatibility (not mentioned in CHANGES.txt for 1.6 ), because response_factory can not be Response class now.

[mmerickel]

The impl of _get_response_factory is identical to this code and defaults to Response if the user has not registered their own factory. I see no incompatibility here.

[mmerickel]

I think maybe what you are referring to is changing the factory to accept the request object. Unfortunately IResponseFactory was a private API prior to 1.6 so we do not need to document those internal changes.

[marc1n]

Yes, I used in 1.5 IResponseFactory in order to set custom Response class. It seems there was no other way.

How could I know that is private and how should I set custom Response class in 1.5?

[mmerickel]

Custom responses classes were not officially supported in 1.5. Pyramid documents all public APIs in the API section of the documentation.

Note the difference from http://docs.pylonsproject.org/projects/pyramid/en/1.5-branch/api/interfaces.html to http://docs.pylonsproject.org/projects/pyramid/en/1.6-branch/api/interfaces.html with regards to IResponseFactory.