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

[Claude Paroz]

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'

[Marten Kenbeek]

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.

[Claude Paroz]

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.

[Tim Graham]

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?

[Claude Paroz]

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.

[Tim Graham]

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.

[Tim Graham]

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

[Tim Graham]

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

[Claude Paroz]

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

[Tim Graham <timograham@…>]

In 6ff356e:

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

[Tim Graham <timograham@…>]

In 075e93c:

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

[Tim Graham <timograham@…>]

In 93d07701:

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

Follow up to 075e93c16a82ba7869a0b2d572e99fdbd0724042.

[Tim Graham <timograham@…>]

In 524f1e70:

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

Backport of 6ff356e891502f3da20f51299f45f4b95d72edd5 from master

[Tim Graham <timograham@…>]

In 2bd152b4:

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

Backport of 075e93c16a82ba7869a0b2d572e99fdbd0724042 and
93d07701045c242f81396016ab4ae15ba63a55d9 from master