Skip to content

Fix type of data parameter in get test requests#267

Merged
intgr merged 4 commits into
typeddjango:masterfrom
nils-van-zuijlen:fix/apirequestfactory-data-type
Feb 22, 2023
Merged

Fix type of data parameter in get test requests#267
intgr merged 4 commits into
typeddjango:masterfrom
nils-van-zuijlen:fix/apirequestfactory-data-type

Conversation

@nils-van-zuijlen

Copy link
Copy Markdown
Contributor

Lists of two-tuples of strings are allowed in the data attribute for get requests

Example of a false-positive needing this fix:

        factory = ApiRequestFactory()
        request = factory.get(users_list_url, data=[("manager", manager1.username), ("manager", manager2.username)])

@nils-van-zuijlen

Copy link
Copy Markdown
Contributor Author

The import paths seem to be wrong in the original code, hence why the tests are broken.

@intgr

intgr commented Nov 8, 2022

Copy link
Copy Markdown
Contributor

The CI has been fixed

@nils-van-zuijlen nils-van-zuijlen force-pushed the fix/apirequestfactory-data-type branch from 6b76c3e to 72624b6 Compare November 21, 2022 14:03
@intgr intgr force-pushed the fix/apirequestfactory-data-type branch from 72624b6 to 9f4bc38 Compare December 3, 2022 11:05
@intgr

intgr commented Dec 3, 2022

Copy link
Copy Markdown
Contributor

CI fixed again, sorry! I rebased your branch.

@sobolevn sobolevn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@intgr intgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR

Comment thread rest_framework-stubs/test.pyi Outdated
@intgr

intgr commented Dec 3, 2022

Copy link
Copy Markdown
Contributor

A similar change would be helpful in django-stubs as well for the RequestFactory class and django.utils.http.urlencode function.

@intgr

intgr commented Dec 21, 2022

Copy link
Copy Markdown
Contributor

The lint and test are failing because an import is missing for Mapping, Iterable

But typecheck fails due to changes in upstream DRF tests; I'll fix that in a bit.

@intgr

intgr commented Dec 21, 2022

Copy link
Copy Markdown
Contributor

From typecheck output

drf_source/tests/test_pagination.py:546: error: Argument 2 to "get" of "APIRequestFactory" has incompatible type "Dict[str, int]"; expected "Union[Mapping[str, Union[str, bytes, Iterable[Union[str, bytes]]]], Iterable[Tuple[str, Union[str, bytes, Iterable[Union[str, bytes]]]]], None]"

Maybe we should accept int as well? It's weird to pass URL params as integer, but on the other hand, this does work and is covered by unit tests.

If there are still remaining typecheck errors after that, we should just suppress them via IGNORED_ERRORS.

Lists of two-tuples of strings are allowed

Co-authored-by: Marti Raudsepp <marti@juffo.org>
@intgr intgr force-pushed the fix/apirequestfactory-data-type branch from f920e73 to 973b707 Compare January 26, 2023 21:27

@intgr intgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I've neglected this project a bit while I focused on django-stubs. Rest assured, I haven't forgotten about it. :)

@intgr

intgr commented Jan 26, 2023

Copy link
Copy Markdown
Contributor

I rebased your branch, there are some false positives in CI typecheck which shoud probably be added to ignores list.

@intgr intgr self-assigned this Jan 27, 2023

@intgr intgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed the relevant typecheck ignores and this can now be merged. Thanks!

@intgr intgr merged commit a038db8 into typeddjango:master Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants