Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30698 closed Cleanup/optimization (wontfix)

`BaseDetailView` and `SingleObjectMixin` optimization.

Reported by: Davit Gachechiladze Owned by: Davit Gachechiladze
Component: Generic views Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Davit Gachechiladze)

Hi. In BaseDetailView.get(self, request, *args, **kwargs), we have line like context = self.get_context_data(object=self.object). Why should we pass object=self.object ? It's redundant (IMHO).

class SingleObjectMixin(ContextMixin):

    def get_context_data(self, **kwargs):
        """Insert the single object into the context dict."""
        context = {}
        if self.object:
            context['object'] = self.object
            context_object_name = self.get_context_object_name(self.object)
            if context_object_name:
                context[context_object_name] = self.object
        context.update(kwargs)
        return super().get_context_data(**context)
class BaseDetailView(SingleObjectMixin, View):
    """A base view for displaying a single object."""
    def get(self, request, *args, **kwargs):
        self.object = self.get_object()
        context = self.get_context_data(object=self.object)
        return self.render_to_response(context)

I think, it's better idea to implement SingleObjectMixin.get_context_data(self, **kwargs) and BaseDetailView.get(self, request, *args, **kwargs) like this. Code below

class SingleObjectMixin(ContextMixin):
    def get_context_data(self, *, object=None, **kwargs):
        """Insert the single object into the context dict."""
        object = object if object is not None else self.object
        context = {}
        if object:
            context['object'] = object
            context_object_name = self.get_context_object_name(object)
            if context_object_name:
                context[context_object_name] = object
        context.update(kwargs)
        return super().get_context_data(**context)
class BaseDetailView(SingleObjectMixin, View):
    """A base view for displaying a single object."""
    def get(self, request, *args, **kwargs):
        self.object = self.get_object()
        context = self.get_context_data()
        return self.render_to_response(context)

Also, SingleObjectMixin.get_context_data(self, *, object=None, **kwargs) will be implemented in a same fashion as MultipleObjectMixin.get_context_data(self, *, object_list=None, **kwargs) is this moment.

https://github.com/django/django/pull/11657

Change History (18)

comment:1 by Davit Gachechiladze, 5 years ago

Description: modified (diff)
Has patch: set

comment:2 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:3 by Davit Gachechiladze, 5 years ago

Owner: changed from nobody to Davit Gachechiladze
Status: newassigned

comment:4 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:5 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:6 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:7 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:8 by Davit Gachechiladze, 5 years ago

Version: 2.2master

comment:9 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:10 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:11 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:12 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:13 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:14 by Davit Gachechiladze, 5 years ago

Description: modified (diff)

comment:15 by Mariusz Felisiak, 5 years ago

Davit please don't update ticket description so many times, you can refer PR in a comment.

in reply to:  15 comment:16 by Davit Gachechiladze, 5 years ago

Replying to felixxm:

Davit please don't update ticket description so many times, you can refer PR in a comment.

Thank you. I will try to do everything better in future.

comment:17 by Carlton Gibson, 5 years ago

Resolution: wontfix
Status: assignedclosed

Hi Davit,

Thanks for the suggestion. Strictly you're right, in theory this simplification is available. We can't make it, however.

Firstly, they'll be plenty of folks that overrode get_context_data() relying on the fact that self.object is injected.
(We change it, we break their code.)

Then, removing relies too much on internal knowledge of the CBV implementations.

The general pattern for get_context_data() is this:

def get_context_data(self, **kwargs):
    context = {
        # Some stuff...
    }
    
    # Allow caller to inject the values we want...
    context.update(kwargs)

    return context

Without looking at SingleObjectMixin at all, the implementation of BaseDetailView.get() is exactly what one want's: we want the calling function to drive `get_context_data() by passing in the object to be rendered.

I don't know if you've seen them but check out http://django-vanilla-views.org/ for a stripped back take on GCBVs here.

in reply to:  17 comment:18 by Davit Gachechiladze, 5 years ago

Replying to Carlton Gibson:

Hi Davit,

Thanks for the suggestion. Strictly you're right, in theory this simplification is available. We can't make it, however.

Firstly, they'll be plenty of folks that overrode get_context_data() relying on the fact that self.object is injected.
(We change it, we break their code.)

Then, removing relies too much on internal knowledge of the CBV implementations.

The general pattern for get_context_data() is this:

def get_context_data(self, **kwargs):
    context = {
        # Some stuff...
    }
    
    # Allow caller to inject the values we want...
    context.update(kwargs)

    return context

Without looking at SingleObjectMixin at all, the implementation of BaseDetailView.get() is exactly what one wants: we want the calling function to drive `get_context_data() by passing in the object to be rendered.

I don't know if you've seen them but check out http://django-vanilla-views.org/ for a stripped back take on GCBVs here.

Thank you for explanation.

Last edited 5 years ago by Carlton Gibson (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top