Address class #227
Merged
Address class #227
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds a new Address NamedTuple class, which we can later use in multiple places, even pass it to functions which take a pure tuple of (host, port), since it is a subclass of tuple. This new class also includes both synchronous and asynchronous methods for performing A record DNS lookups to get the actual IP from a host. This can sometimes be useful, for example to know what ip type the address has (IPv4/IPv6) which is important for making sockets.
ItsDrike
added
area: API
Pyright 1.1.224 has changed it's default behavior, which previously had strictParameterNonevalue set to false, which meant if a default value was None, and the type-hint of that parameter wasn't optional, it was inferred. This change requires Optional to be stated explicitly.
While there's nothing technically wrong with Connection clases taking simple tuples of (host, port), doing that skips address validation checks, and gives us less freedom to work with later.
Only passing host to _retry_query functions means we need to construct the address in there, however this is inefficient since this function is expected to run multiple times and the address will stay the same in every run. Even though creating Address instances is relatively fast, since it's just construction of stdlib's typing.NamedTuple with additional check, there's no need to do it multiple times anyway.
For some reason, when running tests on macos, DNS A record resulution is failing with NXDOMAIN when trying to resolve for 'localhost', instead of properly returning '127.0.0.1'. This issue wasn't previously catched, before the DNS resolving implementation was catching this NXDOMAIN as a way to handle for host already being an IP. This is a temporary fix which adds try-except for NXDOMAIN and sets the IP to host (just like in the previous implementation), which bypasses this failure. But this should be removed as soon as we find the root cause of this mac-os test failure (I don't suppose mac-os actually can't resolve 'localhost' to IP, though it would seem that way from the workflow).
Since tests for SRV resolution were moved to address tests, the server tests no longer covered the lookup method, so this adds tests to explicitly test the lookup constructor only
We used __new__ here because NamedTuple didn't allow using __init__ directly, however since then, we've moved from directly using a class inheriting from NamedTuple to a class inheriting from base class, and only that base class inherits directly from NamedTuple. This means we can actually override __init__ here.
ItsDrike
added
status: needs review
| def const_coro(value: T) -> Callable[..., Awaitable[T]]: | ||
| """This is a helper function, which returns an async func returning value. | ||
|
|
||
| This is needed because in python 3.7, Mock.return_value didn't properly cover |
There was a problem hiding this comment.
Oh yeah, I remember this pain. I used to install an async test helper to have AsyncMock.
👍 for avoiding a dependency with a single function.
kevinkjt2000
approved these changes
Mar 21, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds an
Addressclass, which is aNamedTuplewith multiple custom functionalities related to addresses moved under a single place.This
Addresswill automatically perform the validity checks on initialization, removing the need for us to manually callensure_validfunction each time we initialize the server class, assuming we construct this class there (or take it as a param). It also makes it easy to validate the address on mutations, since Address class, being a subclass of immutable tuple type, you can't mutate it at all, instead you'd need to make a new address to set theself.addressattribute, and again, there are validity checks on initialization, giving us some piece of mind about it.Because this is a subclass of
NamedTuple, it allows us to pass this class to functions which only take a tuple of(host, port), which is quite common, this makes things a bit nicer since it would only allow us to pass addresses passing the validity checks and it's simply neater to dosome_func(self.address)thansome_func((self.host, self.port)).Ideally, I'd like to deprecate the
hostandportattributes of both of the server classes in favor of anaddressatribute holding this new class. This deprecation will probably be done using properties with deprecation notices, returningself.address.hostandself.address.portrespectively, though I'd like to use the deprecated decorator from not yet merged #222. But if it's deemed useful enough, these can stay as non-deprecated properties too, I can see people wanting an easy way to get host and port directly, instead of having to doaddress.host/address.port, if that's desired, please mention it in readme/comment/on discord.Closes #211
Blocked by:
deprecateddecoratorlocalhostinto an IP (127.0.0.1) - Temporarily addressed by 04a8b0a - Note that this isn't a fix, just a way around the problem so that the tests will pass. I didn't put more effort into fixing this since it's not a problem related to this PR and it was there before the DNS logic was improved, it was just getting ignored by an overly wideexceptstatement.