Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33372 closed Bug (invalid)

GISModelAdmin.gis_widget_kwargs cannot be used due to signature mismatch.

Reported by: Nick Pope Owned by: Alexander Filimonov
Component: GIS Version: 4.0
Severity: Normal Keywords: gismodeladmin, gis_widget_kwargs
Cc: Claude Paroz Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.

Change History (9)

comment:1 by Nick Pope, 3 years ago

Easy pickings: set
Has patch: set
Needs tests: set
Patch needs improvement: set

comment:2 by Alexander Filimonov, 3 years ago

Owner: changed from nobody to Alexander Filimonov
Status: newassigned

in reply to:  description ; comment:3 by Mariusz Felisiak, 3 years ago

Cc: Claude Paroz added
Has patch: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: invalid
Severity: Release blockerNormal
Status: assignedclosed

Replying to 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.

in reply to:  3 ; comment:4 by Nick Pope, 3 years ago

Replying to 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.

in reply to:  4 comment:5 by Mariusz Felisiak, 3 years ago

Replying to 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.

in reply to:  4 comment:6 by Alexander Filimonov, 3 years ago

Replying to Nick Pope:

Replying to 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

comment:7 by Mariusz Felisiak, 3 years ago

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.

comment:8 by Alexander Filimonov, 3 years ago

Resolution: invalid
Status: closednew

Please have a look at the pr - little fix will make it more obvious :-)

Last edited 3 years ago by Alexander Filimonov (previous) (diff)

comment:9 by Mariusz Felisiak, 3 years ago

Resolution: invalid
Status: newclosed

Please don't reopen closed ticket. The fix is backward compatible and it introduces unnecessary limitation.

Version 0, edited 3 years ago by Mariusz Felisiak (next)
Note: See TracTickets for help on using tickets.
Back to Top