cbv_comments.rst

Mixins

• A ton of Mixins look really reusable, but do we really need all of them (or any of them).
  • Does is makes sense to have ProcessFormView and DisplayFormView ? Maybe this should be one view (having both functions is the common case) and people can prevent display/process by subclassing.

  • Most view's MRO looks like this: FooBarView, MixinA, MixinB, ..., TemplateMixin, View. If I wanted an almost exactly same view, but a one that uses diffrent template system (or a JSON serializer), I have to either create a new view with same Mixins except for the TemplateMixin; or subclass the FooBarView:

    class JSONFooBar(JSONOverrideMixin, FooBarView): 
        pass

    which defeats the point of having a TemplateMixin, IMHO.

  • Methods in mixins shouldn't raise errors when they don't have implementation. Best option: use the abc.abstractmethod(); Good option: don't provide the method; Medium option: provide an empty method. Raising an error will break MRO:

    class FormWithMessageMixin(ProcessFormMixin): 
    # messages about form validity only make sense, if there is a form to process
    
        def form_valid(self, *args, **kwargs):
            messages.success(self.request, "Action successful")
            return super(MessageMixin, self).form_valid(*args, **kwargs) 
    
    class MyView(MessageMixin, CreateView): # will work as expected
        pass
    
    class MyView(MessageMixin, SomeWeirdView): 
        # if SomeWeirdView doesn't inherit form ProcessFormMixin, exception is raised
        pass

• This isn't a common pattern in Python. Simple cases are ok, but when trouble starts (like above), explaning the MRO to newbies is like teaching them about metaclasses.

get_* methods

• This methods look like accessors. It's uncommon in python to have get_* functions. Using properties would be far better. Unfortunately, this will clash with defaults stored on the class. Only solution that comes to mind involves a metaclass:

  class BaseView(object):
      __metaclass__ = ViewMeta # some magic here
  
  class View(BaseView):
      class Options:
          form_class = None # this will have a default accessor
          model = None
  
       @property
       def form_class(self):
           return MyForm if request.user else super(View, self).form_class

  This is a bit hackery and complex, so I think we can live with get_* syntax ("practicality beats purity.").

• Should get_* methods change the logic flow ? DateView.get_dated_queryset() can raise Http404. If I decide to override this method a more detailed exception (like EmptyPage) would be better.

• Current get_* methods get called exactly once in a predefined point in the logic flow. Their results are then passed as arguments to next get_*. This makes the API very fragile (it's hard to pass more objects). It's also non-obvious why should some methods get certain attributes. For example, in:

  class InstanceFormMixin(FormMixin, SingleObjectMixin):
      """
      A mixin that displays a form for a single object instance.
      """
  
      def get_form(self):
          """
          Returns a form instantiated with the model instance from get_object().
          """
          if self.request.method in ('POST', 'PUT'):
            return self.get_form_class()(
                self.request.POST,
                self.request.FILES,
                initial=self.initial,
                instance=self.get_object(*self.args, **self.kwargs),
             )
         else:
            return self.get_form_class()(
                initial=self.initial,
                instance=self.get_object(*self.args, **self.kwargs),
            ) 

  The get_form_class() can't use the object to determine the form_class. It could, if the object was first retrieved and then passed in. But then the Mixin will fail with classes that don't need the information about object in question. One solution is to have *args, **kwargs signature on all those methods. Another is to store the object information on self. The above code mixes both, which isn't very good.

• In the case above, there is not clean way to add extra parameters that should be passed to Form constructor (so you can use a RequestForm).

Response classes

While overriding render_to_response() or get_response() provides a clean way to override the Response instance if the view is successful, most error conditions just assume that HttpResponse* is fine.

I don't know if it's a very common use case and should Django support it in generic views: My application does almost all HTML rendering on the client side. Django views return JSON, which is then used to render templates in JS. Having HTML error pages doesn't make sense for my application, so I would like to have JSON in all 4xx responses that describe the problem.