Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"field" and "exclude" arguments of model_to_dict() do not accept sets #637

Closed
mthuurne opened this issue Jun 8, 2021 · 1 comment · Fixed by #640
Closed

"field" and "exclude" arguments of model_to_dict() do not accept sets #637

mthuurne opened this issue Jun 8, 2021 · 1 comment · Fixed by #640
Labels
bug Something isn't working

Comments

@mthuurne
Copy link
Contributor

mthuurne commented Jun 8, 2021

Bug report

What's wrong

Our test suite contains code that simplifies to this fragment:

from typing import Mapping

from django.db.models.base import Model
from django.forms import model_to_dict


def check(instance: Model, data: Mapping[str, object]) -> None:
    assert data == model_to_dict(instance, fields=data.keys())

When checking that with mypy, it reports:

testcase.py:8: error: Argument "fields" to "model_to_dict" has incompatible type "AbstractSet[str]";
expected "Union[List[Union[Callable[..., Any], str]], Sequence[str], Literal['__all__'], None]"
[arg-type]
        assert data == model_to_dict(instance, fields=data.keys())

How is that should be

The implementation of model_to_dict() only needs __bool__() and __contains__() to be provided by the fields and exclude arguments, so passing a keys set should not be flagged as an error.

I think a solution could be to replace Sequence in the stubs annotation with Collection.

System information

  • OS: Ubuntu Linux 18.04
  • python version: 3.6.9
  • django version: 2.2.1
  • mypy version: 0.812
  • django-stubs version: 1.8.0
@mthuurne mthuurne added the bug Something isn't working label Jun 8, 2021
@sobolevn
Copy link
Member

sobolevn commented Jun 8, 2021

@mthuurne please, feel free to send a PR! 👍

mthuurne added a commit to ProtixIT/django-stubs that referenced this issue Jun 11, 2021
…peddjango#637)

There are several functions and classes in `django.forms.models` that
take a `fields` or `exclude` argument. Previously, `Sequence` was used
to annotate these, but the code of Django (I checked version 3.2.4)
doesn't require `__getitem__()` to be implemented, so requiring
`Collection` instead is sufficient.

The practical advantage of requiring `Collection` is that a set, such
as the key set of a dictionary, can be passed without first having to
convert it to a list or tuple.
sobolevn added a commit that referenced this issue Jun 11, 2021
…) (#640)

* Allow Collection for 'fields' and 'exclude' of form model helpers (#637)

There are several functions and classes in `django.forms.models` that
take a `fields` or `exclude` argument. Previously, `Sequence` was used
to annotate these, but the code of Django (I checked version 3.2.4)
doesn't require `__getitem__()` to be implemented, so requiring
`Collection` instead is sufficient.

The practical advantage of requiring `Collection` is that a set, such
as the key set of a dictionary, can be passed without first having to
convert it to a list or tuple.

* Pin mypy to below version 0.900

* Remove Callable for 'fields' and 'exclude' of form model helpers

I cannot find any support for callables for these two arguments in
the code or in the documentation.

* Update setup.py

Co-authored-by: Nikita Sobolev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

2 participants