Skip to content

Fix _TYPE_HOSTS to require covariant Sequence #2325

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

Merged
merged 1 commit into from
Oct 5, 2023
Merged

Fix _TYPE_HOSTS to require covariant Sequence #2325

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions elasticsearch/_sync/client/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
List,
Mapping,
Optional,
Sequence,
Set,
Tuple,
Type,
Expand Down Expand Up @@ -69,7 +70,9 @@
# Default User-Agent used by the client
USER_AGENT = create_user_agent("elasticsearch-py", __versionstr__)

_TYPE_HOSTS = Union[str, List[Union[str, Mapping[str, Union[str, int]], NodeConfig]]]
_TYPE_HOSTS = Union[
str, Sequence[Union[str, Mapping[str, Union[str, int]], NodeConfig]]
]

_TYPE_ASYNC_SNIFF_CALLBACK = Callable[
[AsyncTransport, SniffOptions], Awaitable[List[NodeConfig]]
Expand Down Expand Up @@ -139,7 +142,7 @@ def hosts_to_node_configs(hosts: _TYPE_HOSTS) -> List[NodeConfig]:
"""Transforms the many formats of 'hosts' into NodeConfigs"""

# To make the logic here simpler we reroute everything to be List[X]
if not isinstance(hosts, (tuple, list)):
if isinstance(hosts, str):
Comment on lines -142 to +145
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is required because hosts is now a Sequence which allows more than just tuple and list. I considered changing the check to isinstance(hosts, collections.abc.Sequence), but it felt wrong as Python embraces duck typing, which means being a subclass of collections.abc.Sequence is not needed to be an actual sequence. (It was much more convincing when it was still in my head! The irony being that we're doing this to fix type hints where all types are very much explicit.)

Anyway, due to the above, I then decided to switch from a denylist to an allowlist, since only two types currently work: str and NodeConfig. Why disallow NodeConfig then? Because the type does not allow it and I'm not expecting anyone to actually be passing hosts=NodeConfig(...) as it currently only works by accident. (Note that hosts=[NodeConfig(...)] still works.)

return hosts_to_node_configs([hosts])

node_configs: List[NodeConfig] = []
Expand Down