Opened 8 years ago

Closed 8 years ago

#27919 closed Bug (fixed)

Decide if attrs (and possibly others) are named or positional parameters in new widget rendering code

Reported by: Claude Paroz Owned by: Tim Graham
Component: Forms Version: 1.11
Severity: Release blocker Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Discussed on IRC:

claudep: in the new form widget rendering code, we often have attrs=None in signatures, but then passed as positional argument in various calls
claudep: I wonder if this is a good practice
claudep: this can lead to some issues when overriding methods and useing *args, or **kwargs
claudep: thoughts?
MarkusH: claudep: I didn't look at the code, but from what you say I can imagine that leading to issues

A concrete example:

class CustomSelect(Select):
    def create_option(self, name, value, label, selected, index, attrs=None, **kwargs):
        if 'somestring' in label:
            if attrs is None:
                attrs = {}
            attrs['disabled'] = True
        return super().create_option(name, value, label, selected, index, attrs=attrs,  **kwargs)

leads to

...
  File ".../django/forms/widgets.py", line 671, in get_context
    context = super(Select, self).get_context(name, value, attrs)
  File ".../django/forms/widgets.py", line 627, in get_context
    context['widget']['optgroups'] = self.optgroups(name, context['widget']['value'], attrs)
  File ".../django/forms/widgets.py", line 599, in optgroups
    attrs=attrs,
TypeError: create_option() got multiple values for argument 'attrs'

Change History (15)

comment:1 by Marten Kenbeek, 8 years ago

Your method signature is missing the 6th positional argument subindex. Instead you have attrs as the 6th positional argument, so it receives both a positional argument (which should actually be the subindex argument) and a keyword argument.

comment:2 by Claude Paroz, 8 years ago

Yes, I know the reason of the error. But when I read the method signature and see subindex=None, attrs=None, I'm interpreting those as keyword arguments, hence my wrong subclass implementation.

comment:3 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

It looks like the calling code in ChoiceWidget.optgroups() should be self.create_option(..., subindex=subindex, ...) rather than passing subindex as a position argument. Would that solve the issue?

comment:4 by Claude Paroz, 8 years ago

Yes, that was the question. I think this happens at several other places, with attrs and/or subindex. In my experience, it's better to always pass arguments with a default value as keyword arguments, but I didn't find yet any reference supporting that feeling.

comment:5 by Tim Graham, 8 years ago

My guess is the current code is the result of some refactoring along the way. As far as I saw, create_option() is only called in that one spot, so presumably subindex could be an arg instead, if there isn't a use case for calling the method without it. I would say use your best judgment and clean things up as you see fit.

comment:6 by Tim Graham, 8 years ago

Claude, do you plan to propose a patch before Monday's release candidate? If not, I'll look at this.

comment:7 by Tim Graham, 8 years ago

Owner: changed from nobody to Tim Graham
Status: newassigned

comment:8 by Tim Graham, 8 years ago

Has patch: set

PR -- possibly more could be done but I think that PR covers the public APIs.

comment:9 by Claude Paroz, 8 years ago

Triage Stage: AcceptedReady for checkin

Thanks Tim, I'm sorry not being able to be more active at this stage of the 1.11 release.

comment:10 by Tim Graham <timograham@…>, 8 years ago

In 6ff356e:

Refs #27919 -- Passed ChoiceWidget.create_option() kwargs as expected.

comment:11 by Tim Graham <timograham@…>, 8 years ago

In 075e93c:

Refs #27919 -- Changed Widget.get_context() attrs kwarg to an arg.

comment:12 by Tim Graham <timograham@…>, 8 years ago

In 93d07701:

Refs #27919 -- Changed contrib widget's get_context() attrs kwarg to an arg.

Follow up to 075e93c16a82ba7869a0b2d572e99fdbd0724042.

comment:13 by Tim Graham <timograham@…>, 8 years ago

In 524f1e70:

[1.11.x] Refs #27919 -- Passed ChoiceWidget.create_option() kwargs as expected.

Backport of 6ff356e891502f3da20f51299f45f4b95d72edd5 from master

comment:14 by Tim Graham <timograham@…>, 8 years ago

In 2bd152b4:

[1.11.x] Refs #27919 -- Changed Widget.get_context() attrs kwarg to an arg.

Backport of 075e93c16a82ba7869a0b2d572e99fdbd0724042 and
93d07701045c242f81396016ab4ae15ba63a55d9 from master

comment:15 by Tim Graham, 8 years ago

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top