Provide Unpack Args & Improved typehints #180
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #180 +/- ##
==========================================
+ Coverage 97.69% 97.77% +0.07%
==========================================
Files 3 3
Lines 564 583 +19
Branches 38 39 +1
==========================================
+ Hits 551 570 +19
Misses 7 7
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…o added new version of winloop that I dropped yesterday
|
I forgot I upgraded my own libraries lol. Fixed now. |
|
@Dreamsorcerer Would it be ok if I added pre-commit mypy to this library? |
|
I don't generally use pre-commit, so not something I'd make the decision on. |
|
Please don't add a pre commit hook. They make rebasing a real pain. |
@saghul Thanks for letting me know. I will try to find another workaround to the problem I'm having. |
|
|
@Dreamsorcerer I fixed it. Figured out the real cause was not providing |
| self.resolver = aiodns.DNSResolver(loop=self.loop, timeout=5.0) | ||
| self.resolver = aiodns.DNSResolver( | ||
| loop=self.loop, timeout=5.0 | ||
| ) # type[aiodns.DNSResolver | None] |
There was a problem hiding this comment.
Why this change? Also, annotations shouldn't be defined with comments anymore.
| @overload | ||
| def __init__( | ||
| self, | ||
| nameservers: Optional[Sequence[str]] = None, | ||
| loop: Optional[asyncio.AbstractEventLoop] = None, | ||
| ) -> None: ... |
There was a problem hiding this comment.
What's this overload for? The below ones are compatible with it, so this appears redundant.
There was a problem hiding this comment.
Overload requires that it is used twice there was no way I could get around this one.
There was a problem hiding this comment.
@Dreamsorcerer I did have the idea of merging all the arguments together and merging it like this do you think that change might be better? This way we can get rid of some unwanted arguments like socket_state_cb and on the bonus-side of things type hints in this case are retained and nothing tedious has to be done the only thing would be passing these arguments in but then I could get rid of the overloads and Unpack variables all together.
class DNSResolver:
def __init__(
self,
nameservers: Optional[Sequence[str]] = None,
loop: Optional[asyncio.AbstractEventLoop] = None,
*,
flags: Optional[int] = None,
timeout: Optional[float] = None,
tries: Optional[int] = None,
ndots: Optional[int] = None,
tcp_port: Optional[int] = None,
udp_port: Optional[int] = None,
domains: Optional[Sequence[str]] = None,
lookups: Optional[str] = None,
socket_send_buffer_size: Optional[int] = None,
socket_receive_buffer_size: Optional[int] = None,
rotate: bool = False,
local_ip: Optional[str] = None,
local_dev: Optional[str] = None,
resolvconf_path: Optional[str] = None,
) -> None:
self._closed = True
self.loop = loop or asyncio.get_event_loop()
if TYPE_CHECKING:
assert self.loop is not None
self._timeout = timeout
self._event_thread, self._channel = self._make_channel(
flags=flags,
tries=tries,
ndots=ndots,
tcp_port=tcp_port,
udp_port=udp_port,
domains=domains,
lookups=lookups,
socket_send_buffer_size=socket_send_buffer_size,
socket_receive_buffer_size=socket_receive_buffer_size,
rotate=rotate,
local_ip=local_ip,
local_dev=local_dev,
resolvconf_path=resolvconf_path
)
There was a problem hiding this comment.
There was a problem hiding this comment.
We should be dropping 3.9 now and upgrading the type hints anyway..
There was a problem hiding this comment.
Sure thing... Let me see what I can do to finish this off...
|
@saghul @webknjaz @Dreamsorcerer Do I have permission to drop 3.9 here or should I retain backwards support for it? |
|
Normally, it's a good idea to support older Python in the deps (smaller libs) until after the framework (aiohttp) drops it. |
Alright because I just added |
bdraco has already done it. Wait for that PR to merge. Unfortunately, the pycares upgrade looks complex, so will probably have conflicts to resolve here afterwards.
We've dropped it in the repo some time ago, though it only applies to the next 3.14 release. Shouldn't cause any issues to drop it here. |
@Dreamsorcerer I am also upgrading the cyares library under the same upgrade although it seems to use dataclasses rather than the old traditional methods. Although I spent about 5 hours changing code, it will need some more time for testing and ensuring everything runs correctly. Hopefully this will give me insight on how to approach pycares and aiodns when it's finally finished Vizonex/cyares#40 |
4 participants
What do these changes do?
This was apparently part of a todo as I was working on modifying my new library cyares a bit I decided to go on a side-quest to try and type hint as many of these as I could or at least all the keywords that users should use (and not the unitentional ones like socket_cb). I did above and beyond the request by reserving backwards compatibility for earlier versions of python by providing it an overload feature as well. I added a new TODO that involves typehinting all Union[...] and Optional[...] values to using pipes instead so that the annotations can be a bit cleaner and with 3.9 slowly reaching end of life in a couple of months I see no reason not to migrate to newer type hints soon.
Are there changes in behavior for the user?
With newer
__init__keyword arguments now being properly type hinted at, it should help to avoid some unwanted surprises for newer users.Related issue number
Checklist