[Django] #33372: GISModelAdmin.gis_widget_kwargs cannot be used due to signature mismatch.
Django
-------------------------------------+-------------------------------------
Reporter: Nick Pope | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 4.0
Severity: Release | Keywords: gismodeladmin,
blocker | gis_widget_kwargs
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
This issue was raised in https://github.com/django/django/pull/15207.
The problem is that the `gis_widget_kwargs` attribute of
`GeoModelAdminMixin` is `**`-unpacked into `gis_widget` which is
`OSMWidget` by default.
`OSMWidget` and `BaseGeometryWidget` have `.__init__(self, attrs=None)`,
not `.__init__(self, **kwargs)`, so this is broken.
Marking as a release blocker as this is a bug in a new feature introduced
in 4.0.
--
Ticket URL: <https://code.djangoproject.com/ticket/33372>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Nick Pope | Owner: nobody
Type: Bug | Status: new
Component: GIS | Version: 4.0
Keywords: gismodeladmin, | Triage Stage:
gis_widget_kwargs | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Nick Pope):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
* easy: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33372#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Nick Pope | Owner:
Type: Bug | Status: assigned
Component: GIS | Version: 4.0
Severity: Release blocker | Resolution:
Keywords: gismodeladmin, | Triage Stage:
gis_widget_kwargs | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => iGeophysix
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33372#comment:2>
Django
-------------------------------------+-------------------------------------
| Filimonov
Type: Bug | Status: closed
Component: GIS | Version: 4.0
Severity: Normal | Resolution: invalid
Keywords: gismodeladmin, | Triage Stage:
gis_widget_kwargs | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* severity: Release blocker => Normal
* cc: Claude Paroz (added)
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
* has_patch: 1 => 0
* resolution: => invalid
Comment:
Replying to [ticket:33372 Nick Pope]:
> `OSMWidget` and `BaseGeometryWidget` have `.__init__(self, attrs=None)`,
not `.__init__(self, **kwargs)`, so this is broken.
>
> Marking as a release blocker as this is a bug in a new feature
introduced in 4.0.
I wouldn't say it's broken, you can set `gis_widget_kwargs = {'attrs':
{...}}`. Defining widgets with more than `attrs` or a different signature
is reasonable (e.g. `SplitDateTimeWidget` or `ChoiceWidget`). IMO we
should leave wide support for all options.
--
Ticket URL: <https://code.djangoproject.com/ticket/33372#comment:3>
Django
-------------------------------------+-------------------------------------
| Filimonov
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: gismodeladmin, | Triage Stage:
gis_widget_kwargs | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Nick Pope):
Replying to [comment:3 Mariusz Felisiak]:
> I wouldn't say it's broken, you can set `gis_widget_kwargs = {'attrs':
{...}}`. Defining widgets with more than `attrs` or a different signature
is reasonable (e.g. `SplitDateTimeWidget` or `ChoiceWidget`). IMO we
should leave wide support for all options.
I see. That makes sense, but I can also see that it is confusing.
Perhaps the documentation can be updated with an example? (If only to
avoid reports of this in the future.)
In addition, this is not tested - it should be to ensure that
`gis_widget_kwargs` continues to work as expected.
--
Ticket URL: <https://code.djangoproject.com/ticket/33372#comment:4>
Django
-------------------------------------+-------------------------------------
| Filimonov
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: gismodeladmin, | Triage Stage:
gis_widget_kwargs | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Replying to [comment:4 Nick Pope]:
> Perhaps the documentation can be updated with an example? (If only to
avoid reports of this in the future.)
Sounds reasonable. Maybe a short example with changing `default_lon` and
`default_lat`.
> In addition, this is not tested - it should be to ensure that
`gis_widget_kwargs` continues to work as expected.
Extra tests coverage is always warm welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/33372#comment:5>
Django
-------------------------------------+-------------------------------------
| Filimonov
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: gismodeladmin, | Triage Stage:
gis_widget_kwargs | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Alexander Filimonov):
Replying to [comment:4 Nick Pope]:
> Replying to [comment:3 Mariusz Felisiak]:
> > I wouldn't say it's broken, you can set `gis_widget_kwargs = {'attrs':
{...}}`. Defining widgets with more than `attrs` or a different signature
is reasonable (e.g. `SplitDateTimeWidget` or `ChoiceWidget`). IMO we
should leave wide support for all options.
>
> I see. That makes sense, but I can also see that it is confusing.
> Perhaps the documentation can be updated with an example? (If only to
avoid reports of this in the future.)
> In addition, this is not tested - it should be to ensure that
`gis_widget_kwargs` continues to work as expected.
I'd support Nick here and say that it is not a good way to use a
dictionary to fill a single attribute.
I have a fix for this with a test to check if arguments are passed
correctly
https://github.com/django/django/pull/15211
Please review the pull request
--
Ticket URL: <https://code.djangoproject.com/ticket/33372#comment:6>
Django
-------------------------------------+-------------------------------------
| Filimonov
Type: Bug | Status: closed
Severity: Normal | Resolution: invalid
Keywords: gismodeladmin, | Triage Stage:
gis_widget_kwargs | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
> I'd support Nick here and say that it is not a good way to use a
dictionary to fill a single attribute.
As far as I'm aware Nick agreed that the current implementation makes
sense. Of course, it's worth adding test coverage and a short example in
docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/33372#comment:7>
Django
-------------------------------------+-------------------------------------
| Filimonov
Type: Bug | Status: new
Component: GIS | Version: 4.0
Keywords: gismodeladmin, | Triage Stage:
gis_widget_kwargs | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: invalid =>
--
Ticket URL: <https://code.djangoproject.com/ticket/33372#comment:8>
Django
-------------------------------------+-------------------------------------
| Filimonov
Component: GIS | Version: 4.0
Severity: Normal | Resolution: invalid
Keywords: gismodeladmin, | Triage Stage:
gis_widget_kwargs | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
Please don't reopen closed ticket. The fix is backward compatible and it
introduces unnecessary limitation.
--
Ticket URL: <https://code.djangoproject.com/ticket/33372#comment:9>