fix: allow passing a filterset class to the filterset factory

[b1rger]

I've tried to reuse the variable name terminology from django.forms.models.modelform_factory

Closes: #1631

[carltongibson]

Hi @b1rger — this looks good.

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.

[b1rger]

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

[b1rger]

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")...

[carltongibson]

It's probably worth comparing how modelform_factory does it.

[b1rger]

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

[carltongibson]

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.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (af9f13b) 98.59% compared to head (b590408) 98.59%.
Report is 2 commits behind head on main.

❗ Current head b590408 differs from pull request most recent head 75f0cf2. Consider uploading reports for the commit 75f0cf2 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1636   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files          15       15           
  Lines        1283     1284    +1     
=======================================
+ Hits         1265     1266    +1     
  Misses         18       18           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[b1rger]

ack, the Meta.fields in the passed filterset now trumps the fields

[b1rger]

ack, the Meta.fields in the passed filterset now trumps the fields

Hm, scratch that, i pushed too fast, i'll have to rethink the logic...

[b1rger]

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

[carltongibson]

OK, I think this looks good. Just the one comment.

[carltongibson]

Could you check the typos in the test method names please? (Same with test above too.)

[carltongibson]

Updated in #1644