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

Allow tuple as input of query parameters. #1426

Merged
merged 6 commits into from
Dec 12, 2020

Conversation

SarunasAzna
Copy link
Contributor

In the documentation it is stated that params can be dict, string or two
tuples. This allows to used two tuples. Previously it was possible to
use only tuple inside a list.

In the documentation it is stated that params can be dict, string or two
tuples. This allows to used two tuples. Previously it was possible to
use only tuple inside a list.
@StephenBrown2
Copy link
Contributor

The documentation is not wrong, it states specifically that "a list of two-tuples" is allowed.

params - (optional) Query parameters to include in request URLs, as a string, dictionary, or list of two-tuples.

While a tuple of two-tuples should be fine, I'd like to hear the other maintainers thoughts on it. If accepted, you will also need to update the documentation to include "list or tuple of two-tuples." everywhere query params are mentioned, and adjust the QueryParamTypes signature:

httpx/httpx/_types.py

Lines 34 to 41 in 354c4ca

QueryParamTypes = Union[
"QueryParams",
Mapping[str, Union[PrimitiveData, Sequence[PrimitiveData]]],
List[Tuple[str, PrimitiveData]],
str,
bytes,
None,
]
to include Tuple[Tuple[str, PrimitiveData]], which should fix the currently failing mypy tests.

httpx/_models.py Outdated Show resolved Hide resolved
httpx/_models.py Outdated Show resolved Hide resolved
@SarunasAzna
Copy link
Contributor Author

@StephenBrown2 good point about the docs! So actually I think it is important to mention how I found this thing - at the moment we are moving some API agents from requests to httpx and the test with two tuples failed.

@StephenBrown2
Copy link
Contributor

Ah yes, that is an important note! We certainly want to maintain requests compatibility where possible, so go ahead and make the changes I suggested and the others can weigh in once that's done.

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

👍 LGTM as this is — "sequence" in the docs, (list, tuple) in implementation.

@florimondmanca florimondmanca added the enhancement New feature or request label Dec 12, 2020
@florimondmanca florimondmanca merged commit 8696405 into encode:master Dec 12, 2020
@florimondmanca
Copy link
Member

@SarunasAzna Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants