[Django] #27919: Decide if attrs (and possibly others) are named or positional parameters in new widget rendering code
Django
new widget rendering code
-------------------------------------------+------------------------
Reporter: Claude Paroz | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
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'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27919>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
new widget rendering code
Reporter: Claude Paroz | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:1>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:2>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:3>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:4>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:5>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.11
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Claude, do you plan to propose a patch before Monday's release candidate?
If not, I'll look at this.
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:6>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: Tim Graham
Type: Bug | Status: assigned
Component: Forms | Version: 1.11
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => assigned
* owner: nobody => Tim Graham
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:7>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: Tim Graham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/8217 PR] -- possibly more could be
done but I think that PR covers the public APIs.
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:8>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: Tim
| Graham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* stage: Accepted => Ready for checkin
Comment:
Thanks Tim, I'm sorry not being able to be more active at this stage of
the 1.11 release.
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:9>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: Tim
| Graham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"6ff356e891502f3da20f51299f45f4b95d72edd5" 6ff356e]:
{{{
#!CommitTicketReference repository=""
revision="6ff356e891502f3da20f51299f45f4b95d72edd5"
Refs #27919 -- Passed ChoiceWidget.create_option() kwargs as expected.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:10>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: Tim
| Graham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"075e93c16a82ba7869a0b2d572e99fdbd0724042" 075e93c]:
{{{
#!CommitTicketReference repository=""
revision="075e93c16a82ba7869a0b2d572e99fdbd0724042"
Refs #27919 -- Changed Widget.get_context() attrs kwarg to an arg.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:11>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: Tim
| Graham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"93d07701045c242f81396016ab4ae15ba63a55d9" 93d07701]:
{{{
#!CommitTicketReference repository=""
revision="93d07701045c242f81396016ab4ae15ba63a55d9"
Refs #27919 -- Changed contrib widget's get_context() attrs kwarg to an
arg.
Follow up to 075e93c16a82ba7869a0b2d572e99fdbd0724042.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:12>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: Tim
| Graham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"2bd152b46f608933a8c4c6a3c2689781fc58f4fd" 2bd152b4]:
{{{
#!CommitTicketReference repository=""
revision="2bd152b46f608933a8c4c6a3c2689781fc58f4fd"
[1.11.x] Refs #27919 -- Changed Widget.get_context() attrs kwarg to an
arg.
Backport of 075e93c16a82ba7869a0b2d572e99fdbd0724042 and
93d07701045c242f81396016ab4ae15ba63a55d9 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:14>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: Tim
| Graham
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"524f1e7051a36a633abef756d3284886f22c2340" 524f1e70]:
{{{
#!CommitTicketReference repository=""
revision="524f1e7051a36a633abef756d3284886f22c2340"
[1.11.x] Refs #27919 -- Passed ChoiceWidget.create_option() kwargs as
expected.
Backport of 6ff356e891502f3da20f51299f45f4b95d72edd5 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:13>
Django
new widget rendering code
Reporter: Claude Paroz | Owner: Tim
| Graham
Component: Forms | Version: 1.11
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/27919#comment:15>