Fixed #29750 -- Added View.setup() hook. by francoisfreitag · Pull Request #10427 · django/django

francoisfreitag · 2018-09-22T17:54:39Z

Allows encapsulating logic in View mixins for reuse in child classes.

The standard request processing cycle is:

Concrete view
Mixins
Generic view

Before init(), it was not possible to reuse self attributes set in parent views and mixins from the concrete view, because calling the parent method generates a HttpResponse.

https://code.djangoproject.com/ticket/29750

carltongibson

Hi @francoisfreitag. Thanks for this.

Looks OK. (Still not sure it's quite to my taste but whatever... 🙂)

Yes, I think a doc example is worthwhile. Yes, in the topic page.

The example you give in the description is nice. Except I was surprised to see you still override dispatch() in ImpersonationWithMessageMixin. Obviously that works but I thought the idea here was to allow us to leave dispatch alone? Would it not work equally implementing init() and calling super()? (With correct MRO mixins could rely on attributes being set this way — it'd potentially get tricky but it's up to each to keep their code clean and it's already an "Advanced Topic".)

carltongibson · 2018-10-04T08:59:05Z

docs/ref/class-based-views/base.txt

Do we need a note here saying that you must call super() if overriding? (Or similar... obviously you could do it by hand...)

django/views/generic/base.py

francoisfreitag · 2018-10-04T15:56:42Z

Hi Carlton,

Thank you very much for reviewing the PR and your input.

I was surprised to see you still override dispatch() in ImpersonationWithMessageMixin.

In my mind, init() is responsible for performing view attribute initialization and nothing else. The idea behind using dispatch() in the example is to avoid encouraging developers to perform other tasks in init().

[...] the idea here was to allow us to leave dispatch alone?

Not exactly, the idea is to provide parent views and mixins with a way to abstract logic from child classes, but let children control the response that's generated.
In the example, the child ImpersonationWithMessageMixin must know the user being impersonated, that is set by parent ImpersonationMixin. To execute the parent logic that sets self.user without init(), the child needs to call super().dispatch(), which generates a HttpResponse.

Maybe adding a message is not the best example, the example could be updated to check permissions. For example, we could return a HttpResponseForbidden when not self.user.is_active. That way, an admin can't record interest for inactive users.
Checking access permissions could be a better use of dispatch, and it is simpler (doesn't involve the message framework). I'll update the example before to add it to the documentation.

In the ticket, I had hopes to move all self.object = get_object() to init(), to avoid duplication in view methods, such as BaseUpdateView's get() and post(). However, this is backward incompatible: init() would trigger get_object () earlier in the view lifecycle and may issue DB queries when none were necessary before, because dispatch() may raise a PermissionDenied, then the get() or post() method wouldn't execute get_object().

carltongibson · 2018-10-04T16:04:04Z

I had hopes to move all self.object = get_object() to init(), to avoid duplication in view methods, such as BaseUpdateView's get() and post().

I'd be super -1 on this. The get_object() call is an integral part of the view logic. Moving to init() just obscures the flow. A bit of repetition actually makes for clearer code here. (And in many places TBH. It's quite easy to take DRY too far. IMO)

docs/topics/class-based-views/mixins.txt

carltongibson

I'm still not sure I like the feature. It's fine as implemented though.
I'm a bit worried about the docs example. I think it will encourage people down bad roads, rather than being a hook for that time when you really do need it.

What do others think? (I know you like it @jdufresne 😀)

docs/topics/class-based-views/mixins.txt

jdufresne · 2018-11-07T14:00:16Z

What do others think? (I know you like it @jdufresne)

Yeah, I do like it and I think François has done a great job here. I'm +1 on merging this once the docs meet your approval.

charettes · 2018-11-07T14:09:26Z

What do others think?

My only concern here is the init() method name which doesn't make it clear it isn't directly related to __init__() IMO. Were alternative names such as setup() considered?

jdufresne · 2018-11-07T14:23:09Z

I'm not too picky on the name. If setup() is more intuitive and agreeable, no problem here.

carltongibson · 2018-11-07T14:42:38Z

My concern isn’t really the feature at this point. I wouldn’t go this way, but it’s fine and I can see the utility if it’s the style you want to use. So 👍 there.

I am concerned about the docs example: I think a lot of users take the docs as being normative, as saying do this!; I think unless you really know what you’re doing putting parts of your view logic in dispatch is not something you should do. (You should just keep it in your view function, even if that means a bit of repetition calling stepOne(), stepTwo() and so on.) Thus I think the example is tacitly providing the wrong advice for most people. (A small docs entry highlighting the feature for someone who’s already looking for it would be better I think.)

If no one else is feeling this then I’m happy to let it pass. (It’s quite possible I’m just too old. 🙂) But that’s why I asked for other opinions (because I can’t shake the feeling, having given it time enough for a change of heart.) Happy to merge it, happy to release it, but I’d basically like to recuse myself on making the decision here. Thanks for your help all.

francoisfreitag · 2018-11-10T15:24:02Z

Many thanks @carltongibson for approaching this PR with such an open mind!

I removed the example from topics and added a few lines in the View's init() section. Please let me know if that's closer to what you had in mind.

If there's still doubt around this feature, I am happy to open a discussion on the mailing list. The previous discussion did not reach a consensus and other community members may have different perspective on this hook. I could post the removed example on the mailing list to describe the use case and gather opinions. What do you think?

francoisfreitag · 2018-11-10T15:31:18Z

As for the name, I kept init() because it's short and the hook's purpose is to initializes view attributes. I am happy to change it to setup if it helps understand what this hook can be used for. It's clear that setup() will run before dispatch() and I would not expect setup() to return anything.

init() was the name suggested in the previous mailing list discussion. For a while, I had pre_dispatch() in mind, but that's not entirely what this hook does. It allows parent code to execute before child code, so I dropped that idea. That's the names that made sense to me, I'm definitely open to alternatives.

carltongibson · 2018-11-10T16:06:27Z

Hi @francoisfreitag Thanks for the follow-up.

I don't know that we need to go to the mailing list. Jon like feature, Simon has expressed support, I see the use of it (even if it's not my thing per se) — let's go with it.

Only thing now is the docs. (And maybe a rename.) I like the changes you've made: they're much more in line with how I'd present it.

Allow me to give it another once-over and we'll see about getting it in.

Thanks for effort!

carltongibson

Hi @francoisfreitag.

I pushed some edits in 4cb1243.

Can you have a look? Thanks!

francoisfreitag

Edits look good to me, thank you.

In the end, we won't provide use cases to explain the feature. Maybe we should add the use case for user impersonation suggested in a previous version of this PR to the ticket? That may help future readers understand the use case. What do you think?

django/views/generic/base.py

carltongibson · 2018-11-14T12:34:51Z

My thought was that it's fine as-is, i.e. minus the example. (I think people looking for it can come up with their own use-cases.) There's plenty of discussion on the ticket (I think).

I also thought that your use-case would make a good blog-post, on your own or company blog. (No harming in throwing a link to such on the ticket...)

carltongibson · 2018-11-14T13:10:48Z

Were alternative names such as setup() considered?

So I was just about to mark this RFC when I remembered this. Naming things...

tests/generic_views/test_base.py

timgraham · 2018-11-14T23:36:54Z

django/views/generic/base.py

I think if not hasattr(...) and raising an exception would be more in line with Django's style.

timgraham · 2018-11-14T23:37:34Z

django/views/generic/base.py

You can omit the tuple and just put this on the previous line.

timgraham · 2018-11-14T23:38:08Z

docs/ref/class-based-views/base.txt

timgraham · 2018-11-14T23:38:42Z

docs/ref/class-based-views/base.txt

a, b, and c (missing comma after 'b')

also missing .. versionadded:: 2.2 annotation

Also you mentioned a use case in the release notes but not here.

docs/ref/class-based-views/generic-display.txt

timgraham · 2018-11-14T23:40:39Z

docs/releases/2.2.txt

I think "The new" is more inline with other notes.

View.init <django.views.generic.base.View.init> would render "View" also which would help.

timgraham · 2018-11-14T23:41:59Z

docs/releases/2.2.txt

I would replace "class..." with "view" (no link)

timgraham · 2018-11-14T23:42:17Z

docs/releases/2.2.txt

It allows....

timgraham · 2018-11-14T23:43:45Z

tests/generic_views/test_base.py

Use self.assertRaisesMessage.

timgraham · 2018-11-14T23:44:38Z

tests/generic_views/test_base.py

I would put this in the one test they're used in unless you think they have potential to be used in future tests.

carltongibson · 2018-11-15T06:04:46Z

Hey Tim. Thanks for the review. Do you have any thoughts on the naming question?

timgraham · 2018-11-15T14:54:22Z

No strong opinion. One idea is that it the name init could create some ambiguity with __init__ when speaking.

carltongibson · 2018-11-19T09:20:22Z

No strong opinion...

OK, let's call that a +0.5 🙂 I'll say the same for Simon and myself (for total +1.5)

@francoisfreitag Could you change for Simon's suggestion of setup(). I don't suppose there's much in it but it avoids the verbal confusion as Tim says?

Then if you could address Tim's review comments and rebase I think we're there. 🎉

francoisfreitag · 2018-11-19T09:37:56Z

I don't mind changing to setup(). Thanks for the feedback, I'll update the PR in a few days.

francoisfreitag · 2018-12-02T09:43:30Z

Early commit to address comment "Use self.assertRaisesMessage." introduced as #10714.

Ready for review.

francoisfreitag · 2018-12-12T10:13:27Z

Sorry @timgraham, I push-forced your edits. I restored them from https://github.com/django/django/compare/53d05c3d166eb2f5e410b7a6c2c947a6480b1c50..c1022f47e051924288d2d5cfbdf7c7cfecd59a79, but would not mind a second set of eyes.

I'll make sure to use --force-with-lease in the future.

francoisfreitag · 2018-12-12T10:15:42Z

The commit message was reverted. I was only able to restore the first line.

francoisfreitag · 2018-12-12T10:16:38Z

Note: I did make a change on my side (CheckInitView -> CheckSetupView).

carltongibson

OK. This looks ready to go. Thanks for the effort @francoisfreitag! 🎁

Just the one observation...

carltongibson · 2018-12-20T09:34:43Z

docs/ref/class-based-views/base.txt

I preferred the shorter version we had earlier:

Initializes view instance attributes: ``self.request``, ``self.args`` and ``self.kwargs`` prior to :meth:`dispatch`. Child classes overriding this method must call ``super()``.

I restored it from https://github.com/django/django/compare/53d05c3d166eb2f5e410b7a6c2c947a6480b1c50..c1022f47e051924288d2d5cfbdf7c7cfecd59a79#diff-2df22e647d63040c7db3eca921c888fdL87, I believe this change was part of @timgraham's cosmetic edits.

The longer version explains the purpose of this hook, that's why I have a small preference for it.

…docs.

…n docs. Backport of 734fde7 from master

todd-dembrey · 2021-06-17T15:35:09Z

tests/generic_views/test_base.py

+    def test_not_calling_parent_setup_error(self):
+        class TestView(View):
+            def setup(self, request, *args, **kwargs):
+                pass  # Not calling supre().setup()


s/supre/super/

(was fixed in b916c27)

carltongibson reviewed Oct 4, 2018

View reviewed changes

francoisfreitag force-pushed the 29750_pre_dispatch branch 3 times, most recently from 5a7c1be to 57533c0 Compare October 19, 2018 21:40

carltongibson reviewed Nov 5, 2018

View reviewed changes

docs/topics/class-based-views/mixins.txt Outdated Show resolved Hide resolved

carltongibson reviewed Nov 5, 2018

View reviewed changes

docs/topics/class-based-views/mixins.txt Outdated Show resolved Hide resolved

francoisfreitag force-pushed the 29750_pre_dispatch branch from 57533c0 to 8a31de5 Compare November 10, 2018 15:19

francoisfreitag force-pushed the 29750_pre_dispatch branch from 8a31de5 to 769a761 Compare November 10, 2018 15:24

carltongibson reviewed Nov 14, 2018

View reviewed changes

francoisfreitag commented Nov 14, 2018

View reviewed changes

django/views/generic/base.py Outdated Show resolved Hide resolved

francoisfreitag force-pushed the 29750_pre_dispatch branch from 4cb1243 to 9f7f476 Compare November 14, 2018 13:17

francoisfreitag commented Nov 14, 2018

View reviewed changes

tests/generic_views/test_base.py Outdated Show resolved Hide resolved

francoisfreitag force-pushed the 29750_pre_dispatch branch from 9f7f476 to 0df04c7 Compare November 14, 2018 13:25

timgraham reviewed Nov 14, 2018

View reviewed changes

francoisfreitag force-pushed the 29750_pre_dispatch branch from 0df04c7 to 3a377bd Compare December 2, 2018 09:40

timgraham changed the title ~~Fixed #29750 -- Added init() hook to View~~ Fixed #29750 -- Added View.setup() hook. Dec 6, 2018

timgraham force-pushed the 29750_pre_dispatch branch from 3a377bd to 53d05c3 Compare December 6, 2018 23:27

francoisfreitag force-pushed the 29750_pre_dispatch branch 2 times, most recently from c1022f4 to 0af6de3 Compare December 12, 2018 10:10

francoisfreitag force-pushed the 29750_pre_dispatch branch from 0af6de3 to 8b3dfa3 Compare December 12, 2018 10:14

carltongibson approved these changes Dec 20, 2018

View reviewed changes

Fixed #29750 -- Added View.setup() hook for class-based views.

e671337

timgraham force-pushed the 29750_pre_dispatch branch from 8b3dfa3 to e671337 Compare December 22, 2018 00:08

timgraham merged commit e671337 into django:master Dec 22, 2018

francoisfreitag deleted the 29750_pre_dispatch branch December 26, 2018 10:03

felixxm pushed a commit to smithdc1/django that referenced this pull request Apr 10, 2020

Refs django#10427 -- Corrected BoundField.css_classes() signature in …

734fde7

…docs.

felixxm pushed a commit that referenced this pull request Apr 10, 2020

[3.0.x] Refs #10427 -- Corrected BoundField.css_classes() signature i…

eeab4ab

…n docs. Backport of 734fde7 from master

todd-dembrey reviewed Jun 17, 2021

View reviewed changes

Uh oh!

Conversation

francoisfreitag commented Sep 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

francoisfreitag commented Oct 4, 2018

Uh oh!

carltongibson commented Oct 4, 2018

Uh oh!

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jdufresne commented Nov 7, 2018

Uh oh!

charettes commented Nov 7, 2018

Uh oh!

jdufresne commented Nov 7, 2018

Uh oh!

carltongibson commented Nov 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francoisfreitag commented Nov 10, 2018

Uh oh!

francoisfreitag commented Nov 10, 2018

Uh oh!

carltongibson commented Nov 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

francoisfreitag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carltongibson commented Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carltongibson commented Nov 14, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carltongibson commented Nov 15, 2018

Uh oh!

timgraham commented Nov 15, 2018

Uh oh!

carltongibson commented Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

francoisfreitag commented Sep 22, 2018 •

edited

Loading

carltongibson commented Nov 7, 2018 •

edited

Loading

carltongibson commented Nov 10, 2018 •

edited

Loading

carltongibson commented Nov 14, 2018 •

edited

Loading

carltongibson commented Nov 19, 2018 •

edited

Loading

francoisfreitag Dec 20, 2018 •

edited

Loading