Opened 3 weeks ago
Closed 2 weeks ago
#37364 closed Cleanup/optimization (wontfix)
Make NullBooleanSelect.format_value() return a list, consistent with ChoiceWidget
| Reported by: | Brian Helba | Owned by: | Brian Helba |
|---|---|---|---|
| Component: | Forms | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Brian Helba | Triage Stage: | Unreviewed |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
ChoiceWidget.format_value() is documented (in its docstring) to "Return selected values as a list", and every ChoiceWidget subclass honors that, except NullBooleanSelect, which returns a bare string: "unknown", "true", or "false".
NullBooleanSelect does not override get_context() or optgroups(). It relies on the inherited ChoiceWidget.optgroups(), which marks an option as selected with str(subvalue) in value. When value is a list that is list membership. When value is the bare string it becomes a substring test, which only produces correct results because none of "unknown", "true", or "false" happens to be a substring of another. The parent's contract is satisfied by accident rather than by design, and it may be confusing for anyone who later customizes the choices or the parent implementation.
History: #27866 (1.11) changed ChoiceWidget.format_value() from returning a set to returning a list but did not touch NullBooleanSelect. #17210 (2.2) changed the NullBooleanSelect mapping from 1/2/3 to unknown/true/false and kept returning a bare string.
Proposed change: wrap the result in a one-element list, i.e. ["unknown"], ["true"], or ["false"]. The rendered HTML should be identical. The only impact would be for third-party code that calls widget.format_value() directly and uses the result as a value representation.
Change History (2)
comment:1 by , 3 weeks ago
| Has patch: | set |
|---|---|
| Owner: | set to |
| Status: | new → assigned |
comment:2 by , 2 weeks ago
| Resolution: | → wontfix |
|---|---|
| Status: | assigned → closed |
Looking at django-stubs nicely shows the inconsistency here
# NullBooleanSelect.format_value violates the Liskov substitution principle
@override
def format_value(self, value: Any) -> str: ... # type: ignore[override]
However, as it has been like this _forever_ any change now would be a breaking change. Here is one public example:
...
def format_value(self, value):
if value in [None, "", "None"]:
return [""]
return [super().format_value(value)]
I don't think that making the code consistent now is worth the churn for users so I'll close as wontfix. If you'd like to discuss further, the next step would be to raise it on the django forum to seek wider consensus with the community.
https://github.com/django/django/pull/22009