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

Remove incorrect str possibility from HostnameResolver.getaddrinfo #2964

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

tjstum
Copy link
Member

@tjstum tjstum commented Feb 27, 2024

_socket.getaddrinfo takes care of encoding all passed in str host arguments before passing it to a custom resolver. Additionally, the docs say that a resolver will always receive IDNA-encoded bytes.

However, the type hints of HostnameResolver.getaddrinfo imply that a resolver may need to contend with a str

Remove the extraneous appearance

`_socket.getaddrinfo` takes care of encoding all passed in `str` host
arguments before passing it to a custom resolver. Additionally, the
docs say that a resolver will always receive IDNA-encoded bytes.

However, the type hints of `HostnameResolver.getaddrinfo` imply that a
resolver may need to contend with a `str`

Remove the extraneous appearance
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.64%. Comparing base (accaae4) to head (adcf46f).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2964   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files         117      117           
  Lines       17634    17634           
  Branches     3172     3172           
=======================================
  Hits        17572    17572           
  Misses         43       43           
  Partials       19       19           
Files Coverage Δ
src/trio/_abc.py 100.00% <ø> (ø)
...c/trio/_tests/test_highlevel_open_tcp_listeners.py 100.00% <ø> (ø)
src/trio/_tests/test_highlevel_open_tcp_stream.py 100.00% <ø> (ø)
src/trio/_tests/test_highlevel_ssl_helpers.py 100.00% <ø> (ø)
src/trio/testing/_fake_net.py 100.00% <ø> (ø)

Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

I agree with this assessment. For others who want to double check:

trio/src/trio/_socket.py

Lines 223 to 230 in accaae4

if isinstance(host, str):
try:
host = host.encode("ascii")
except UnicodeEncodeError:
# UTS-46 defines various normalizations; in particular, by default
# idna.encode will error out if the hostname has Capital Letters
# in it; with uts46=True it will lowercase them instead.
host = _idna.encode(host, uts46=True)

... I was going to ask about putting @typing.override on these implementations of HostnameResolver but turns out that doesn't warn on the signatures being different! So nevermind.

Copy link
Member

@mikenerone mikenerone left a comment

Choose a reason for hiding this comment

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

Pretty straightforward, seems correct.

@A5rocks A5rocks merged commit 13ee1b0 into python-trio:master Feb 29, 2024
28 checks passed
@jakkdl
Copy link
Member

jakkdl commented Feb 29, 2024

thanks! Surprised there haven't been more typing errors that snuck through.

@CoolCat467
Copy link
Contributor

Type checker must be working well!

@tjstum
Copy link
Member Author

tjstum commented Mar 4, 2024

Thanks everyone!

I realize I forgot a changelog entry. Does this warrant one, or is it too small to care?

@jakkdl
Copy link
Member

jakkdl commented Mar 4, 2024

Thanks everyone!

I realize I forgot a changelog entry. Does this warrant one, or is it too small to care?

I guess it technically is a public-facing change, but I think it's probably too small to care.

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

Successfully merging this pull request may close these issues.

None yet

5 participants