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.

Sign up for GitHub

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

Jump to bottom

Support Enum query params #2515

Closed
wants to merge 4 commits into from
Closed

Support Enum query params #2515

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c5a884
Support Enum based on query params
Kludex Dec 26, 2022
b583641
Merge branch 'master' into feat/query-params-enum
Kludex Dec 30, 2022
98e5ad7
Merge branch 'master' into feat/query-params-enum
tomchristie Jan 4, 2023
7a33454
Merge branch 'master' into feat/query-params-enum
tomchristie Jan 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions httpx/_urls.py
View file
Open in desktop
Expand Up @@ -6,7 +6,7 @@
import rfc3986.exceptions

from ._exceptions import InvalidURL
from ._types import PrimitiveData, QueryParamTypes, RawURL, URLTypes
from ._types import QueryParamTypes, RawURL, URLTypes
from ._utils import primitive_value_to_str


Expand Down Expand Up @@ -554,7 +554,6 @@ def __init__(

value = args[0] if args else kwargs

items: typing.Sequence[typing.Tuple[str, PrimitiveData]]
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

This was unused.

if value is None or isinstance(value, (str, bytes)):
value = value.decode("ascii") if isinstance(value, bytes) else value
self._dict = parse_qs(value, keep_blank_values=True)
Expand Down
3 changes: 3 additions & 0 deletions httpx/_utils.py
View file
Open in desktop
@@ -1,5 +1,6 @@
import codecs
import email.message
import enum
import logging
import mimetypes
import netrc
Expand Down Expand Up @@ -67,6 +68,8 @@ def primitive_value_to_str(value: "PrimitiveData") -> str:
return "false"
elif value is None:
return ""
elif isinstance(value, enum.Enum):
return str(value.value)
Comment on lines +71 to +72
Copy link
Member

Choose a reason for hiding this comment

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

Does this imply that the PrimitiveData type ought to change to include enum.Enum?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Yeah, I miss that.

Do you think that enum.Enum should not be considered a primitive data, and hence not be in this function?

Copy link
Member

Choose a reason for hiding this comment

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

Honest answer?
My preference would be for us to raise nice tight TypeError cases here.

Copy link
Member

Choose a reason for hiding this comment

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

For a similar example...

import json

json.dumps{"value": Color.RED}        # TypeError: Object of type Color is not JSON serializable
json.dumps{"value": Color.RED.value}  # '{"value": 1}'

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

The thing is... I think the following should raise an error:

class Color(enum.Enum):
    RED = 1
    GREEN = 2
    BLUE = 3

But not the following:

class Color(int, enum.Enum):
    RED = 1
    GREEN = 2
    BLUE = 3

Because the int there means that each element is an integer, analogous to other types.

Copy link
Member

Choose a reason for hiding this comment

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

Hrm. That's a bit awkward...

class Color(str, enum.Enum):
    # heeeeyyyyyy I'm just a normal str
    RED = "red"
    GREEN = "green"
    BLUE = "blue"

c = Color.RED
isinstance(c, str)  # True
str(c)              # 'Color.RED'  yeah but you ain't

The upshot is even if you're type-checking the user still needs to correctly pass Color.RED.value, unless we special-case for enum.

Maybe the upshot is we should have strict type checking here and that we should also expand the primitive types to include enum.Enum. (Just because we're friendly like that?)

(Or maybe the upshot is that users should pass the enum value, because we're cold hard calculating types, and we like precision)

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

The upshot is even if you're type-checking the user still needs to correctly pass Color.RED.value, unless we special-case for enum.

If I understood correctly what you said, this is not the case. Type checkers understand that each member of a (str, Enum) is a str, and they don't raise errors.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I don't think requests has the same behavior as httpx here, does it? Because the Testclient changed, so I assume requests can do the "right" thing on (str, Enum) - I'm on my phone.

return str(value)
Copy link
Member

Choose a reason for hiding this comment

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

I think I'd be happier if HTTPX had an extra branch if isinstance(value, str): return value here, and then raised an explicit TypeError for all remaining cases.

For Enum, we could assume that people want value.value to be used, but why assume and potentially cause unexpected behavior when we can make users provide exactly what they want with a few keystrokes?

Is my thinking, at least

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

The if isinstance(value, str): return value solves the issue that I want to solve here.

Should I proceed? Aren't we going to have some side effects?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

It was kind of what happened on #2523, and we had to revert.



Expand Down
12 changes: 12 additions & 0 deletions tests/client/test_queryparams.py
View file
Open in desktop
@@ -1,3 +1,5 @@
import enum

import httpx


Expand All @@ -22,6 +24,16 @@ def test_client_queryparams_string():
assert client.params["a"] == "b"


def test_client_queryparams_enum():
class MyEnum(str, enum.Enum):
A = "a"
B = "b"

client = httpx.Client(params={"a": MyEnum.A})
assert isinstance(client.params, httpx.QueryParams)
assert client.params["a"] == "a"


def test_client_queryparams_echo():
url = "http://example.org/echo_queryparams"
client_queryparams = "first=str"
Expand Down