Support as_view as a protocol in path()

Django Internals
1

The explicit call to .as_view() for CBVs seems a bit awkward to me. We could change _path to add:

if hasattr(view, 'as_view'):
    view = view.as_view()

As it’s a protocol type check, this would also benefit iommi as we use the same as_view() call.

1 Like
2

See #21595 (Automatically call as_view() when urlpatterns encounter a CBV.) – Django.

Russel’s comment 10 is the essence of it:

This was considered and rejected as part of the original CBV work. See the wiki for the rationale.

Short version: it’s all about keeping a simple contract for urlpatterns.

3

I read through that as best as I could, but the most I found was that it was suggested and then not replied to.

I don’t personally buy the argument of keeping the path() contract simple. There is already a check for isinstance(view, View), that throws an error instead of doing the right thing. I do think that the check of isinstance instead of hasattr() is a bad idea though as it limits the helpfulness of the error to just subtypes of View.

I think we can have a new discussion and not be stuck in the old decision. That was a very different time, when CBVs were just proposed. We now have a situation where CBVs are recommended as the standard way to write views.

4

I think changing the usage here would cause more trouble than it’s worth.

Path &co expect a callable view function. Allowing folks to pass in other things confuses that. It’s not (IMO) a net-win.

5

CBVs are not recommended as the standard way to write views. In fact I personally recommend the opposite as it’s more “Pythonic” – simple is better than complex :man_shrugging:

6

@shangxiao don’t start that in this thread :laughing:

I am +1 for the proposal, despite being in camp FBV. I have always found typing .as_view() confusing, like: “Why am I converting my View to a view? I thought it already was one?” I believe beginners also find this confusing, and even long-time users, at least myself, sometimes forget to type it.

Using a protocol as proposed is good for other classy view things and has a similar consistency to the ORM’s as_sql protocol.

1 Like
7

The code in path() would be simpler actually. We delete:

    elif isinstance(view, View):
        view_cls_name = view.__class__.__name__
        raise TypeError(
            f"view must be a callable, pass {view_cls_name}.as_view(), not "
            f"{view_cls_name}()."
        )

and instead replace it with:

if hasattr(view, 'as_view'):
    view = view.as_view()

Simpler, and more semantically clear.

8

This code should not be removed, since the as_view method cannot be called on View instances, that piece of code is also a guard for missuse.

An effective check would first ensure that the view is a type and a subclass of View before calling the classmethod.

9

Having as_view means folks have to address the view callable issue and get clear on it. All the existing docs, tutorials and examples show this usage.

Instead the proposal is to move from one to multiple ways to do this, with the new way being implicit rather than explicit.

1 Like
10

That’s incorrect:

from django.views import View


class Foo(View):
    pass


Foo().as_view()

If you run this:

  File "/[...]/venv/lib/python3.11/site-packages/django/utils/decorators.py", line 11, in __get__
    raise AttributeError(
AttributeError: This method is available only on the class, not on instances.

The check is redundant.

11

hmm… actually, the single use @classonlymethod makes my suggestion of if hasattr(...) not work, because the hasattr call will cause a raise and then return false. Classic foot gun of hasattr.

So I will change my suggestion to:

view = resolve_as_view(view)

with this helper:

def resolve_as_view(view):
    try:
        as_view = view.as_view
    except AttributeError:
        return view
    return as_view()

which is a bit annoying to read, but keeps @classonlymethod intact, implements the protocol cleanly and simplifies the code overall.

12

I am all for keeping the specific and helpful error message in there instead of relying on the more generic error message from classonlymethod.

I also like the idea of auto-handling the default case of MyView.as_view() within urlpatterns, while still allowing to call it explicitly e.g. to pass custom initkwargs if needed or to use it in e.g. tests to convert the CBV to a callable outside of urlpatterns.

On a quick glance, there are 29 uses of .as_view() in our code base, 27 of those are the normal no-args case within a urlpattern. I’d find it easier to read and understand if passing a CBV directly would work.

2 Likes
13

As someone who rarely uses django CBV’s, I’m all for what @boxed proposes. I remember last time I had to use it, it was like:
I passed FooView
→ error
→ “Ah, I still need that classmethod, what was the name… ah as_view”
→ error
→ “hm, I need to call it… why is it so complicated”

So, if people don’t need it most of the times, why not make it easier? :person_shrugging: