Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30689 closed Bug (invalid)

Fix documentation for the `fields` argument of MultiValueField

Reported by: Adam Sołtysik Owned by: nobody
Component: Documentation Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the documentation there is the class header:

class MultiValueField(fields=(), **kwargs)

However, in the source code the fields argument has no default value:

def __init__(self, fields, *, require_all_fields=True, **kwargs):

This behaviour seems to have changed in Django 2.0 (some of my forms started crashing after the upgrade), but there is nothing about it in the release notes.

Change History (4)

comment:1 by Carlton Gibson, 5 years ago

Resolution: invalid
Status: newclosed
Type: UncategorizedBug
Version: 2.2master

Without actual code it's difficult to know what's broken in your project but I'd guess the relevant change was #28192. Optional args are now keyword only. As per the note in the Backwards incompatible changes section of the 2.0 release notes:

To help prevent runtime errors due to incorrect ordering of form field arguments, optional arguments of built-in form fields are no longer accepted as positional arguments.

In general, the reference docs do not attempt to match the code signature of the documented methods exactly. Rather they aim to give a guide to the interface from the user perspective. In this case, fields doesn't have an actual default, but is a required tuple of fields, as per the discussion below.

comment:2 by Adam Sołtysik, 5 years ago

My problem was that the fields argument was not passed to the constructor of a class inheriting from MultiValueField. In Django 1.11 it worked, because empty tuple () was the default for this argument, just like in the class header in the documentation, and since Django 2.0 there is no default value. The exact code change is here: 8e752d84378c7169ef73a483ffffcba55a08c867.

I think this is a breaking change that should be documented in the release notes. This:

Form fields no longer accept optional arguments as positional arguments

is not relevant, since fields is in fact accepted as a positional argument.

Last edited 5 years ago by Carlton Gibson (previous) (diff)

comment:3 by Carlton Gibson, 5 years ago

8e752d84378c7169ef73a483ffffcba55a08c867 was just correcting a bug: that fields was not in fact required, contrary to the documentation.

Sorry that you've hit this, but it doesn't qualify as a breaking change. (All bugfixes are breaking changes if you're relying on the broken behaviour...)

comment:4 by Adam Sołtysik, 5 years ago

I get it, though I think there's a difference between a bug when something doesn't work, and a "bug" when something works but it maybe shouldn't. That's the second time I stumbled upon such a situation -- several days ago I was refactoring part of the project because of #30333, and the change was also not documented. What's sad is that it's not my fault, as I didn't work on the project from the beginning and I didn't write the code in question, but now I have no way of being sure whether nothing else is broken after the upgrade. So maybe it's worth considering to add a special section to release notes just for code-breaking bugfixes ;)

Note: See TracTickets for help on using tickets.
Back to Top