Closed b1rger closed 7 months ago
Could you add a test or two using the new argument? Specifically, interested in the base filter set defining some properties to be reused, fields and meta options: inheritance of meta options needs testing. Also we should doc this.
done
FTR: currently the Meta.fields
attribute of the base filterset is always overwritten by the fields
that is passed to the factory (which defaults to ALL_FIELDS
) - that means if you want to provide use a custom filterset class together with custom field definitions, you would have to both provide a custom filtersetclass and pass the fields to the factory. It might be more usable if the fields
argument of the factory got only used if the base filterset Meta class does not have one. Or if the fields
argument of the factory is None, but that would break the method signature... maybe only set fields if fields is None and not hasattr(filterset.Meta, "fields")
...
It's probably worth comparing how modelform_factory
does it.
It's probably worth comparing how
modelform_factory
does it.
They are passing fields=None
to the factory and then:
attrs = {"model": model}
if fields is not None:
attrs["fields"] = fields
I can update the code accordingly, if its ok to change the fields
argument default from ALL_FIELDS
to None
They're subclassing the Meta
class, so that defined options are inherited.
But yes, as long as we maintain the effective existing behaviour where fields
wasn't passed, with no base FormSet class, then we should be able to adjust.
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
af9f13b
) 98.59% compared to head (b590408
) 98.59%. Report is 2 commits behind head on main.:exclamation: Current head b590408 differs from pull request most recent head 75f0cf2. Consider uploading reports for the commit 75f0cf2 to get more accurate results
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
ack, the Meta.fields
in the passed filterset
now trumps the fields
ack, the
Meta.fields
in the passedfilterset
now trumps thefields
Hm, scratch that, i pushed too fast, i'll have to rethink the logic...
Oke, I think I found an approach that should not lead to surprises: if the fields
argument is not None
it overrides everything. If it is None
, the Meta.fields
of the filterset argument is used, if it exists and is not None
. If thats also not the case, fields
is set to ALL_FIELDS
Updated in #1644
I've tried to reuse the variable name terminology from
django.forms.models.modelform_factory
Closes: #1631