Recent changes to test.client.Client.get() do not accept valid data types

[jmichalicek]

Bug report

What's wrong

test.client.Client.get() expects the data arg to be Mapping[str, str | bytes | Iterable[str | bytes]] | Iterable[tuple[str, str | bytes | Iterable[str | bytes]]] | None but the method works with other options as well such as when the value is a bool. ie. self.client.get(url, data={'somevar': True}

How is that should be

get() data with a bool value should be accepted.

Here is an example from the codebase in question

class QueryArgs(TypedDict):
            query_type: str
            only_selected: bool
            ingredients: list[str]

class TestParams(TypedDict):
            query_args: QueryArgs  # Feels like I should be able to just define the TypedDict inline here
            # query_args: Union[Mapping[str, Union[str, bytes, Iterable[Union[str, bytes]]]], Iterable[Tuple[str, Union[str, bytes, Iterable[Union[str, bytes]]]]], None]
            expected: QuerySet[Recipe]

test_matrix: list[TestParams] = [
            {
                "query_args": {
                    "query_type": "any",
                    "only_selected": False,
                    "ingredients": [self.rum.slug, self.orange_curacao.slug],
                }
                ,
                "expected": Recipe.objects.all(),
            },
]

# there are actually more tests in the matrix
for t in test_matrix:
            with self.subTest(t["query_args"]):
                query_args = t["query_args"]
                response = self.client.get(self.url, data=query_args)
                self.assertEqual(200, response.status_code)
                self.assertQuerysetEqual(t["expected"], response.context["object_list"])

This works with django-stubs 1.13.1, but fails with 1.13.1 and 1.13.2 even though self.client.get() works as expected and translates the python True and False correctly.

System information

• OS:
• python version: 3.11.1
• django version: 4.1.5
• mypy version: 0.991
• django-stubs version: 1.13.2
• django-stubs-ext version: 0.7.0

[intgr]

Most likely caused by #1297, pinging @nils-van-zuijlen

[intgr]

self.client.get(url, data={'somevar': True}

I know this works. But these values will be converted to a URL query string and thus stringified anyway. It won't retain the original bool data type after deserialization (unlike for example JSON). So passing booleans or ints into query params seems just sloppy.

Personally I prefer my type stubs to be stricter, even if it means I have to adapt my code and make a few changes here and there.

The adaptation is straightforward, just quote the boolean value: self.client.get(url, data={'somevar': 'True'}

Do you see any downsides doing this? Any third opinions?

[nils-van-zuijlen]

As the author of this type annotation, I am aware it is not perfect.

It was mentioned in the PR that ints should be allowed because there are unit tests in Django that use ints, and I fully agree that it is a useful pattern (having a parameterized test that loops through a range). Int parameters are annotated in the django-rest-framework sister PR typeddjango/djangorestframework-stubs#267

I personally don't think booleans should be allowed because there is no normalized way to represent them in urls. (to mind comes 0 or 1, false or true, f or t, `` or on, upper and lower case, etc.) So I do think str should be used instead of bools for on/off values. One could use `str(some_bool)` if the default python representation is the one you want to use.
To annotate these, you can use Literal as of python 3.8.

The question becomes, do we want to annotate with all working types, or with only the types that make sense?

[intgr]

Int parameters are annotated in the django-rest-framework sister PR

Yeah, that got omitted from the django-stubs one.

I think since Python's bool is a subtype of int, allowing int would also automatically allow bool?

As stated, my slight preference is not to allow them. But I don't have strong feelings either way.

What's more important is that we are consistent between django-stubs and djangorestframework-stubs.

[intgr]

Another point is that this only affects test code. I guess it's fine to prioritize convenience over strictness in tests.

[jmichalicek]

Fair enough. My feeling was that if Django expects and handles the types, then the type hints should allow for it.

[intgr]

To clarify by "Another point" I meant as a point to allow int in this context (which AFAICT also allows bool).

@nils-van-zuijlen as the author of these, I'll leave the final decision up to you.

[intgr]

int and bool will be accepted, after PR #1345.