-
Notifications
You must be signed in to change notification settings - Fork 70
Provide Unpack Args & Improved typehints #180
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #180 +/- ##
==========================================
+ Coverage 97.25% 97.34% +0.08%
==========================================
Files 3 3
Lines 547 565 +18
Branches 34 35 +1
==========================================
+ Hits 532 550 +18
Misses 8 8
Partials 7 7 ☔ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this overload for? The below ones are compatible with it, so this appears redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overload requires that it is used twice there was no way I could get around this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
)
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