Opened 12 years ago

Closed 12 years ago

#17148 closed Bug (fixed)

WizardView uses get_context_data incorrectly

Reported by: bradley.ayers@… Owned by: nobody
Component: contrib.formtools Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

https://code.djangoproject.com/browser/django/trunk/django/contrib/formtools/wizard/views.py#L538

However docs for every other generic view say get_context_data only takes kwargs, not args. I think it should be changed to form=form

Attachments (3)

17148.diff (3.6 KB ) - added by Bradley Ayers <bradley.ayers@…> 12 years ago.
17148.alternative.diff (1.4 KB ) - added by Julien Phalip 12 years ago.
trac-17148.diff (6.2 KB ) - added by steph 12 years ago.
Updated patch and extended tests

Download all attachments as: .zip

Change History (18)

comment:1 by bradley.ayers@…, 12 years ago

Component: Uncategorizedcontrib.formtools
Easy pickings: set
Type: UncategorizedBug
Version: 1.3SVN

comment:2 by Julien Phalip, 12 years ago

Triage Stage: UnreviewedAccepted

This is a minor thing, but it makes sense to try and stay consistent here.

comment:3 by Bradley Ayers <bradley.ayers@…>, 12 years ago

It became a problem when I wrote a mixin that expected the kwargs protocol and it didn't work :(

comment:4 by Julien Phalip, 12 years ago

It'd be great if you could provide a test case illustrating that mixin problem. The patch itself should then be trivial.

comment:5 by Bradley Ayers <bradley.ayers@…>, 12 years ago

Something along the lines of:

class NoopMixin(object):
    def get_context_data(self, **kwargs):
        return super(TestMixin, self).get_context_data(**kwargs)


class TestWizard(NoopMixin, WizardView)
    pass
    
    
view = TestWizard.as_view([forms.Form])

comment:6 by Bradley Ayers <bradley.ayers@…>, 12 years ago

Sorry:

class NoopMixin(object):
    def get_context_data(self, **kwargs):
        return super(NoopMixin, self).get_context_data(**kwargs)

comment:7 by Julien Phalip, 12 years ago

Thanks, your tickets are much appreciated. If you could provide patches while your mind is fresh working with formtools, it'd help fast-track all of those changes.

comment:8 by Bradley Ayers <bradley.ayers@…>, 12 years ago

I'll see if I can whip something up in a few hours.

by Bradley Ayers <bradley.ayers@…>, 12 years ago

Attachment: 17148.diff added

comment:9 by Bradley Ayers <bradley.ayers@…>, 12 years ago

Has patch: set

by Julien Phalip, 12 years ago

Attachment: 17148.alternative.diff added

comment:10 by Julien Phalip, 12 years ago

Triage Stage: AcceptedDesign decision needed

I'm not sure that what was originally reported here is necessary a problem. It's OK for a class to define a different signature for a method it overrides. In the context of the WizardView the form is quite important, so it would make sense to make it a required parameter in WizardView.get_context_data()'s signature -- if we decide to do so, then it would be a matter of clarifying the documentation. It could also be argued that consistency should be preserved with every other view and therefore that WizardView.get_context_data()'s signature should also be (self, **kwargs). I haven't personally used formtools in real projects yet so I prefer to leave that decision to someone who has (or perhaps to someone like jezdez who's done a lot of work on this). So for now, I'll switch this ticket to DDN.

I've noticed, however, a related but different issue. TemplateView.get_context_data() only accepts **kwargs, yet it is also passed *args by WizardView.get_context_data(). I have corrected that in the attached patch.

comment:11 by Bradley Ayers <bradley.ayers@…>, 12 years ago

I want to elaborate on the issue I had. I wrote a generic mixin that implements get_context_data(self, **kwargs), and my inheritance was:

class MyView(MyMixin, WizardView):
    pass

I suppose I could have just switched the order of the inheritance. I'm not sure if this would solve every scenario like this though.

comment:12 by Julien Phalip, 12 years ago

I see. The test case in your original patch only contains a single inheritance. If you could provide a test case using some mixins and where the current method signature poses a problem, then that would make the normalisation of signatures more compelling.

comment:13 by Jannis Leidel, 12 years ago

Triage Stage: Design decision neededAccepted

Note the related tickets #16074 and #17242 which I think describe the general problem.

by steph, 12 years ago

Attachment: trac-17148.diff added

Updated patch and extended tests

comment:14 by steph, 12 years ago

I just uploaded a new patch with more tests. I think this is ready for checkin.

comment:15 by Julien Phalip, 12 years ago

Resolution: fixed
Status: newclosed

In [17231]:

Fixed #17148 -- Fixed the signature of WizardView.get_context_data() to play nicer with mixin inheritance. Thanks, Bradley Ayers and Stephan Jaekel.

Note: See TracTickets for help on using tickets.
Back to Top