[django-filter] Replace list with more generic Sequence #14567
Conversation
for FilterSetOptions and filterset_factory
This comment has been minimized.
This comment has been minimized.
|
@huynguyengl99 @intgr Maybe you can review this PR |
| fields: Collection[str] | dict[str, Collection[str]] | str | None | ||
| exclude: Collection[str] | None |
There was a problem hiding this comment.
Should we use Iterable instead of Collection here? I think we just need to iterate over the fields, so Iterable might be a better choice.
There was a problem hiding this comment.
At least for exclude there is a __contains__ check (django_filters.filterset.BaseFilterSet.get_fields L305)). Therefore I switched to Collection.
Also, if one uses an iterator/generator, this may break things since it runs just once.
There was a problem hiding this comment.
Hmm, I checked djangorestframework-stubs, and it looks like the package mainly uses Sequence and does not use Collection anywhere. I think it would be better to use Sequence to follow the current usage in Django REST framework.
|
@H4rryK4ne Oh, sorry — I had already reviewed it but forgot to press Submit 😂 |
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
for FilterSetOptions and filterset_factory
fixes #14565