Views are using a subclass or HttpRequest #141

sebpiq · 2012-01-22T19:33:04Z

The new custom request takes care of ".DATA", ".FILES", overloaded HTTP method, content type and content.

tests
some doc.

…able directly on the request - see #128

tomchristie · 2012-01-22T22:22:00Z

Nice one!
I assume there's some docs that we'd need to change to for, and that the examples would need altering too?
There's some stuff in there that I'll probably change slightly, but this looks really helpful.

sebpiq · 2012-01-23T06:03:25Z

I'll update the examples, and correct the wrong tests. Concerning the docs, I am not really sure where it should be updated. Ideas ?

Also, while I am at it, what would you change ?

tomchristie · 2012-01-23T08:12:18Z

Rather than create a new class at runtime, subclassing from request, I'd instead store the original request object in ._request and delegate any getattr, setattr calls through to the original request object. Then you don't need the request factory stuff.

t.

On 23 Jan 2012, at 06:03, [email protected] wrote:

I'll update the examples, and correct the wrong tests. Concerning the docs, I am not really sure where it should be updated. Ideas ?

Also, while I am at it, what would you change ?

Reply to this email directly or view it on GitHub:
#141 (comment)

sebpiq · 2012-01-23T08:27:51Z

Yep ... that's one of the suggestions I listed here : #128

Like I said there - the solution you suggest feels like the most hackish to me, especially when you need to do a method overriding :

def my_method(self, bla):
    do_stuff()
    self.__getattribute__('my_method')(bla)
    do_more_stuff()

And if it's for doing that, subclassing HttpRequest makes actually no sense ; also you loose the original request class (which can be a WSGIRequest or smthg else ...).
It is also very prone to naughty bugs (I know for having tried this approach many times in other projects).

That said, if you still think it's the best solution ... let's go for it. But you should really weight the pros and cons. And do you really think it'll make for less hackish code ?

tomchristie · 2012-01-23T09:29:47Z

Sorry I hadn't read through that yet. Yup those look like the 3 possibilities to me too.

For overriding existing methods you'd do...

def my_method(self, ...):
    ...
    self._request.my_method(...)
    ...

I'm fairly sure it'll be pretty nice and clean, but if there's any issues happy to rethink.

tomchristie · 2012-01-23T09:32:19Z

Re. docs. in terms of this pull request I'm only really concerned with making sure that any examples that are in there at the moment are updated if they need to be.

sebpiq · 2012-01-23T09:43:43Z

Aw ... yeah, I don't know what I had in mind with that snippet !!! You're absolutely right. On the other hand ... is it just a matter of preference ? Or is there an actual difference between the two solutions ? Because, as it works like that ... if it's just for cosmetics, we can also change it later.

sebpiq · 2012-01-23T09:44:19Z

Yep ... I'll update the examples. What about the doc ?

tomchristie · 2012-01-23T10:20:11Z

Just make sure there's nowhere in the docs that's broken I guess.

On 23 Jan 2012, at 09:44, [email protected] wrote:

Yep ... I'll update the examples. What about the doc ?

Reply to this email directly or view it on GitHub:
#141 (comment)

sebpiq · 2012-01-24T17:20:47Z

Examples didn't need any modification (or so it seemed). I checked, I didn't see any place where request was used directly, so it seems ok to me. Now, to the docs !

tomchristie · 2012-01-24T17:37:25Z

self.CONTENT is used all over the place, which I think we can replace with request.DATA after this change.

tomchristie · 2012-01-24T17:40:46Z

Er, with request.CONTENT I mean. I was jumping ahead a bit too much there.

sebpiq · 2012-01-24T19:13:07Z

Note that in this pull request I didn't touch to .CONTENT.
This is part of ResourceMixin at the moment, and contains the validated content ... should data validation be put in the request (I personally think it shouldn't) ?
Anyway, wherever .CONTENT ends-up, it's for the next step (I mean that I'll take care of it after .DATA is settled).

sebpiq · 2012-01-24T22:12:28Z

And some doc + example in the browsable API

tomchristie · 2012-01-26T19:02:07Z

Looking good!
I'm considering deferring pulling this in until we've done the same for responses too (I might find the time to do that.),
and then very soon after that making the 0.4.0 release.
What do you think?

sebpiq · 2012-01-26T21:24:33Z

I think it sounds good !
About the example (how-to) I wrote, I got a sudden inspiration that since the framework should be (one day) entirely uncoupled, it would be great to have a small topical guide on each feature that can be used individually (that's why I wrote this "how-to integrate enhanced requests to your custom views"). What do you think about that ?

… yet.

… renamed ErrorResponse to ImmediateResponse

sebpiq · 2012-02-07T14:55:24Z

Ok @tomchristie . The responses are done too. Now would be a good time to review it, and maybe think of merging it to the trunk at last !!!

tomchristie · 2012-02-08T20:51:18Z

Great! It might take a little while before it gets pulled in - it's such a big backwards-incompatible change that I'd like to really go through it well before it gets released.

There's a few bits & pieces that I'd tweak - How would you like my to tackle that? - Do you want me to review and you make the changes, or would you like me to go ahead and make 'em myself. (Aware that you've already put a lot of time into this, so don't want to ask too much!)

tomchristie · 2012-02-09T20:37:06Z

djangorestframework/mixins.py

-            raise ErrorResponse(status.HTTP_404_NOT_FOUND,
-                                {'detail': 'That page contains no results'})
+            raise ImmediateResponse(
+                content={'detail': 'That page contains no results'},


Drop the content keyword arg.

tomchristie · 2012-02-09T20:54:19Z

Looking good!
Am looking forward to the point where you can just return a Response from any regular Django view.

sebpiq · 2012-02-10T09:09:02Z

Ok ... I made the suggested fixes. It would be great now if you could pull - so I can get this nice change set off my hands :) I know there are still a bunch of stuff to do, but we can create tickets for those, so that anybody motivated can also do it. For example, the examples - and DocumentingRenderer - are a bit broken, and I don't have time to fix them now.

sebpiq · 2012-02-10T09:13:05Z

I would say - if you do pull - there's 4 stuff still to be done :

write a guide for using Response / ResponseMixin
fix/flesh-out the guide for using Request / RequestMixin
fix the examples (there's a bug in there that I really couldn't figure out)
move format overload out of renderers https://github.com/sebpiq/django-rest-framework/blob/master/djangorestframework/renderers.py#L63

tomchristie · 2012-02-10T13:38:39Z

Yo. Also see my notes on #8 re. request.auth and request.user.

Going to need to start documenting all this pretty soon.

The end goal is that we should end up with 5 very nicely decoupled areas:

request / parsers / authentication
response / renderers / serialization
view
validation
resources & resourceviews

sebpiq · 2012-02-10T14:14:48Z

Hmm you forget to mention a huge bit :

deserialization
validation

And that's part of the problems mentioned in #158.

It's going to be complicated mostly because Django folks have made the choice (bad choice imo), to put together deserialization and validation in forms. And we wanna use forms right !? But I'd really really like those 2 steps to be tweakable separately. And that'll require a lot of niftiness and creativity !!!

sebpiq · 2012-02-10T14:22:19Z

Also I'm not entirely convinced that more stuff should live with request/response. How will you ensure after that everything is uncoupled ? request/parsers and response/renderers make sense because renderers and parsers are useless alone, and response and request are useless alone. But that's not the case for serialization or authentication !!! I would say the parts should be :

request / parsers
authentication
deserialization
validation
resources
serialization
response / renderers

Because all of those are atomic bits that you might want to use separately

tomchristie · 2012-02-10T14:58:21Z

Also I'm not entirely convinced that more stuff should live with request/response.

The auth stuff has to be part of the request, because we have to force request.user to use our flexible per-request authentication, rather than just Django's global authentication backend.

If we don't do that then we end up with both request.user and view.user which makes no sense.
Even if we renamed view.user it's still confusing, because there's no obvious reason to the end user why they shouldn't access request.user directly.

The serialization going onto the response let's three things happen:

Drop this hack: https://github.com/sebpiq/django-rest-framework/blob/b33579a7a18c2cbc6e3789d4a7dc78c82fb0fe80/djangorestframework/views.py#L241
You can throw any kind of object into a response and they'll render sensibly. (Rather than only supporting basic python types)
We can be super awesome and have a TemplateResponse, which renders to html using the given template by default, but can also render to the json/the self describing api/xml/whatever. You'd be able to return either style of response from any regular view in Django, and it's possible to build your entire site which supports all of .html, .json and .api on any view.

I'd really really like those 2 steps to be tweakable separately.

I don't think we need to go into lots of detail about validation/deserialization right now, that's one for another day.

Have faith! ;-p

tomchristie · 2012-02-10T15:11:16Z

NB. I'm more likely to be swayed that the serializers bit might not be needed, but I think the auth part definitely is.

sebpiq · 2012-02-10T16:10:48Z

If we don't do that then we end up with both request.user and view.user which makes no sense.

Absolutely right. In fact, I haven't really looked at the authentication part, so I trust you.
But then it is going to be quite challenging to come up with a good API to still keeps a good separation between the two. Because on one hand they need to be integrated, and on the other hand, what if you want to use only Request ?

I'm totally convinced by 3. !!! I was thinking about that yesterday actually : what if ??? I tried to render all my pages AND json API with the same view from drf ? And that kind of give an answer to it !!! Although now (introducing get_renderers) it is quite easy to achieve the same functionality. So I think, it really makes sense, but once again, it's going to be a challenge to come-up with a good API - and keep a low coupling.

So, yeah ... I keep faith !!!

tomchristie · 2012-02-10T16:59:56Z

Ace!

tomchristie · 2012-02-10T17:00:05Z

(And comments noted.)

sebpiq · 2012-02-11T15:40:50Z

So ... apart from all that, is there still anything holding you from pulling this in ? Should I fix anything else ?

sebpiq · 2012-02-14T08:06:42Z

there is still something I couldn't fix, cause it requires more thinking : #163

tomchristie · 2012-02-17T09:21:27Z

Pulled into develop. :)

sebpiq · 2012-02-17T13:11:10Z

Yaaaay !!! :D

.DATA, .FILES, overloaded HTTP method, content type and content avail…

ab0b72a

…able directly on the request - see #128

corrected request example

8b72b7b

sebpiq added 2 commits January 24, 2012 21:21

documentation for request module

714a90d

enhanced request how-to + example

152c385

sebpiq added 8 commits January 31, 2012 23:21

merge

279fa0d

merged with trunk's master

5f59d90

Response as a subclass of HttpResponse - first draft, not quite there…

5bb6301

… yet.

merge

a0dc0b1

cleaned a bit Response/ResponseMixin code, added some documentation +…

ca96b45

… renamed ErrorResponse to ImmediateResponse

cleaned Request/Response/mixins to have similar interface

21292d3

some docs for Request/Response/mixins

6963fd3

modified examples, somethin' still broken, can't find what

2cdff1b

tomchristie reviewed Feb 9, 2012
View reviewed changes

Sébastien Piquemal added 2 commits February 10, 2012 10:18

made suggested fixes

db0b010

attempt at fixing the examples

b33579a

flashingpumpkin mentioned this pull request Feb 10, 2012

Streamline and unmagicify view.DATA / view.CONTENT #158

Closed

tomchristie mentioned this pull request Feb 10, 2012

Add OAuth authentication/permissions. #8

Closed

fixed examples, corrected small bugs in the process

821844b

merged with trunk

c04cb51

tomchristie closed this Feb 17, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Views are using a subclass or HttpRequest #141

Views are using a subclass or HttpRequest #141

sebpiq commented Jan 22, 2012

tomchristie commented Jan 22, 2012

sebpiq commented Jan 23, 2012

tomchristie commented Jan 23, 2012

sebpiq commented Jan 23, 2012

tomchristie commented Jan 23, 2012

tomchristie commented Jan 23, 2012

sebpiq commented Jan 23, 2012

sebpiq commented Jan 23, 2012

tomchristie commented Jan 23, 2012

sebpiq commented Jan 24, 2012

tomchristie commented Jan 24, 2012

tomchristie commented Jan 24, 2012

sebpiq commented Jan 24, 2012

sebpiq commented Jan 24, 2012

tomchristie commented Jan 26, 2012

sebpiq commented Jan 26, 2012

sebpiq commented Feb 7, 2012

tomchristie commented Feb 8, 2012

tomchristie Feb 9, 2012

tomchristie commented Feb 9, 2012

sebpiq commented Feb 10, 2012

sebpiq commented Feb 10, 2012

tomchristie commented Feb 10, 2012

sebpiq commented Feb 10, 2012

sebpiq commented Feb 10, 2012

tomchristie commented Feb 10, 2012

tomchristie commented Feb 10, 2012

sebpiq commented Feb 10, 2012

tomchristie commented Feb 10, 2012

tomchristie commented Feb 10, 2012

sebpiq commented Feb 11, 2012

sebpiq commented Feb 14, 2012

tomchristie commented Feb 17, 2012

sebpiq commented Feb 17, 2012

Views are using a subclass or HttpRequest #141

Views are using a subclass or HttpRequest #141

Conversation

sebpiq commented Jan 22, 2012

tomchristie commented Jan 22, 2012

sebpiq commented Jan 23, 2012

tomchristie commented Jan 23, 2012

sebpiq commented Jan 23, 2012

tomchristie commented Jan 23, 2012

tomchristie commented Jan 23, 2012

sebpiq commented Jan 23, 2012

sebpiq commented Jan 23, 2012

tomchristie commented Jan 23, 2012

sebpiq commented Jan 24, 2012

tomchristie commented Jan 24, 2012

tomchristie commented Jan 24, 2012

sebpiq commented Jan 24, 2012

sebpiq commented Jan 24, 2012

tomchristie commented Jan 26, 2012

sebpiq commented Jan 26, 2012

sebpiq commented Feb 7, 2012

tomchristie commented Feb 8, 2012

tomchristie Feb 9, 2012

Choose a reason for hiding this comment

tomchristie commented Feb 9, 2012

sebpiq commented Feb 10, 2012

sebpiq commented Feb 10, 2012

tomchristie commented Feb 10, 2012

sebpiq commented Feb 10, 2012

sebpiq commented Feb 10, 2012

tomchristie commented Feb 10, 2012

tomchristie commented Feb 10, 2012

sebpiq commented Feb 10, 2012

tomchristie commented Feb 10, 2012

tomchristie commented Feb 10, 2012

sebpiq commented Feb 11, 2012

sebpiq commented Feb 14, 2012

tomchristie commented Feb 17, 2012

sebpiq commented Feb 17, 2012