Use a single response object

[Changaco]

Reticketed from #222 (comment):

Regarding the Response class, I think we should kill the raise Response(...) pattern. Instead of creating new response objects everywhere, we should only use the one in the state. That would allow us to give it access to the website and request objects, and thus provide a richer API.

[Changaco]

Does anyone have objections? I'd like to do this before Pando 1.0.

[chadwhitacre]

Whether Response is raisable and whether it's global are separable concerns. Are you looking to change one, the other, or both?

[Changaco]

I'm not looking to prevent the raising of response objects, I'm only proposing that we use a single response object per request instead of creating new ones when raising.

[Changaco]

Also, ideally the developer should be able to replace the Response class with a subclass, and that's not possible if we're creating new instances of the base class all over the place.

[Changaco]

To clarify further, this is what I'm trying of get rid of: https://github.com/liberapay/liberapay.com/blob/134/liberapay/main.py#L139-L183

[chadwhitacre]

I'd like to still be able to raise Responses, but I'm fine to pass the class around somehow instead of using it globally as we've been doing.

[chadwhitacre]

that's not possible if we're creating new instances of the base class all over the place.

It's not the creation of new instances that's the problem, but the global import. If we pass the Response class around in state and only instantiate the one pulled from there (rather than from pando import Response) then we could create multiple instances and still be able to control which class is in use.

[Changaco]

Yes, however subclassing Response isn't the primary reason why I want a single instance.

The first issue is that the Response class currently doesn't have access to the website and request objects, though this could be solved by changing raise Response(…) to raise request.Response(…), since there is only one request object and we could give it a reference to website.

The bigger problem is that when you're creating new instances you're throwing away every modification that previous functions may have made to the response object already in the state, such as adding cookies. I've hit this problem in Liberapay, and it seems to me that the best way to fix it is to use a single response object.

[chadwhitacre]

Hmm ... I would expect the opposite: website.Response(request). There's a new request for each request, but the same website. Likewise, there's one Response class, it doesn't change from request to request (does it?).

What does it look like to raise the response from a simplate?

if False:
    response.code = 400
    response.body = b'you bad'
    raise response

or maybe ...

    response.raise_with(400, b'you bad')

?

response(400, 'you bad') looks weird to me. I expect a lowercased callable to be a function, and I expect a function to have a verb for a name.

[Changaco]

response(400, 'you bad') looks weird to me. I expect a lowercased callable to be a function, and I expect function to have a verb for a name.

Then I suggest response.send(…). That function would raise the response, but if you want to make it clear that you're raising you can do raise response.send(…).

[Changaco]

However, when you're doing raise Response(400, 'you bad') you don't actually want to send that response, you know it'll be picked up by delegate_error_to_simplate and another more complete response will be rendered using error.spt or 400.spt. So maybe response.error(…) would be better.

[chadwhitacre]

I do want to make it clear that I'm raising. What about framing it in terms of the request?

    raise request.terminate(400, 'you bad')

The point is that this is as far as the request is going to get. From here on out we're on our way back out to the browser.

v           ^     ^
 \         /     /
  \       /     /
   \     /     /
    \   /     /
     \ /     /
      Y     /
       \   /
        \ /
         V

[chadwhitacre]

Or .end.

[chadwhitacre]

    raise EndOfRequest(400, 'you bad')

[chadwhitacre]

    raise request.End(400, 'you bad')

[chadwhitacre]

I like that last one.

[Changaco]

What about framing it in terms of the request?

I think it's weird. It's like the defunct Request.redirect() function, while it's true that "redirect the request" makes sense, what the function actually does is manipulate a response object, so IMO it belongs in the Response class.

[chadwhitacre]

Okay. Well, we've got a few options out on the table. I'm happy to let you decide. :-)

[Changaco]

It just occurred to me that having a single response object is incompatible with #304.