Skip to content

Add typings - #366

Merged
fantix merged 3 commits into
MagicStack:masterfrom
bryanforbes:add-typings
Feb 9, 2021
Merged

fantix merged 3 commits into
MagicStack:masterfrom
bryanforbes:add-typings

Conversation

@bryanforbes

Copy link
Copy Markdown
Contributor

This PR adds typings for use with type checkers like mypy. Overall, this was fairly easy but two errors have to be suppressed with a # type: ignore comment:

  • remove_signal_handler() returns a bool and typeshed has this method returning None in AbstractEventLoop. This seems to be an error in typeshed since the docs state it should return a bool.
  • create_connection() in Python 3.8+ accepts two new arguments (happy_eyeballs_delay and interleave) and since uvloop.Loop.create_connection() doesn't allow those arguments, the two methods are incompatible. This may be unavoidable.

References #358

Comment thread uvloop/loop.pyi Outdated
def can_write_eof(self) -> bool: ...
def abort(self) -> None: ...

class _Server(asyncio.AbstractServer):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm curious why we need this as opposed to simply using asyncio.AbstractServer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

asyncio.AbstractServer is defined as follows in typeshed:

class AbstractServer:
    sockets: Optional[List[socket]]
    def close(self) -> None: ...
    if sys.version_info >= (3, 7):
        async def __aenter__(self: _T) -> _T: ...
        async def __aexit__(self, *exc: Any) -> None: ...
        def get_loop(self) -> AbstractEventLoop: ...
        def is_serving(self) -> bool: ...
        async def start_serving(self) -> None: ...
        async def serve_forever(self) -> None: ...
    async def wait_closed(self) -> None: ...

Since uvloop works on Python 3.5+ and Server defines those methods for all versions, asyncio.AbstractServer will be wrong for uvloop on Python 3.5 and 3.6.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll be dropping support for 3.5 and 3.6. 3.5 is no longer supported by Python core devs; 3.6 is in security fixes only mode. And with 3.9 now out, uvloop will be 3.7+ only in its next release, supporting three Python versions: 3.7, 3.8, and 3.9.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's good to know! I'll go back over this PR with that in mind.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One thing to note: I think we'll still need _Server because AbstractServer.sockets is Optional[List[Socket]] and Server.sockets is List[Socket].

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should stick to AbstractServer typing here too. That makes it easy for us to fix our Server.sockets property to follow the spec.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Some of these others I added while I was navigating the source. I'll make sure I validate all of these are needed when I go over this again.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, thanks. And we can always fix uvloop code that doesn't conform to asyncio APIs.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry that it's taken so long to get back to this. I've updated the PR based on your feedback and rebased on master. Unfortunately, the PR checks are failing prematurely.

@1st1

1st1 commented Feb 9, 2021

Copy link
Copy Markdown
Member

@fantix We can probably merge this one too, please review.

@bryanforbes

Copy link
Copy Markdown
Contributor Author

I can rebase this on master if it would make it easier to review. Just let me know.

@fantix

fantix commented Feb 9, 2021 •

Copy link
Copy Markdown
Member

@bryanforbes Yes, please! Thank you! Never mind, it's done 😃

@bryanforbes

Copy link
Copy Markdown
Contributor Author

I have a couple of small edits to make before you merge

@fantix fantix left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! I'll be waiting for your edits.

@bryanforbes

Copy link
Copy Markdown
Contributor Author

@fantix I committed and pushed the edits. Let me know if you have questions about them. One thing to note: the two things I pointed out in the description of this PR are still issues. Once those methods are changed, the # type: ignore[misc] can be removed from __init__.py.

@fantix

fantix commented Feb 9, 2021

Copy link
Copy Markdown
Member

Once those methods are changed, the # type: ignore[misc] can be removed from __init__.py.

Sounds good 👍

@fantix
fantix merged commit 9426e2b into MagicStack:master Feb 9, 2021
@1st1

1st1 commented Feb 9, 2021

Copy link
Copy Markdown
Member

Thanks @bryanforbes!

@bryanforbes

Copy link
Copy Markdown
Contributor Author

Not a problem! If you ever have any typing questions, feel free to reach out to me.

@1st1

1st1 commented Feb 9, 2021

Copy link
Copy Markdown
Member

@bryanforbes thanks! If you have time please take a look at MagicStack/immutables#54 -- another our package.

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.

3 participants