numberoverzero / bottom

asyncio-based rfc2812-compliant IRC Client
http://bottom-docs.readthedocs.io
MIT License
74 stars 23 forks source link

Add type annotations #36

Closed miedzinski closed 7 years ago

miedzinski commented 7 years ago

Another take on adding type annotations to bottom.

There are few # type: ignore comments, but that's okay:

    def connection_made(self, transport: asyncio.BaseTransport) -> None:
        assert isinstance(transport, asyncio.Transport)
        self.transport = transport

and we'll have the benefit of being sure that right transport is being used, and mypy will be happy.

Besides that everything is cleanly typechecking.

Currently there is stubs directory with annotations for simplex. If you're okay with this I'd remove this and add typing to simplex repository as well (with py2-compatible syntax).

miedzinski commented 7 years ago

I have changed one type annotation to be more specific and removed one # type: ignore comment with changes suggested in PR description.

If you decide to merge this, please decide what to do with simplex first. If you want to add type annotation there, tell me whether they should be py2 compatible (function annotations or type comments).

numberoverzero commented 7 years ago

Thanks for the PR! I'm +1 for type annotations, but there are a few small artifacts that I'm not a fan of. I'll try to add concrete feedback over the weekend, if things don't come up.

Every time it comes up I dislike the simplex dependency more and more. It's personally useful, but it isn't core to what bottom offers and really is more of an example of using the plugin system. I'd prefer to pull it out entirely, bump major version, drop simplex from requirements, and put it in the Extensions/Plugins docs. Looks like this work would also benefit from that effort.

miedzinski commented 7 years ago

Sure. I dislike simplex too; bottom is quite low-level IRC library (and this is one of the main reasons I love it), but for some reason there is something like Router.

miedzinski commented 7 years ago

@numberoverzero, done. I've changed isinstance(..., Transport) assertion to WriteTransport - we don't need ReadTransport. And since mypy 0.501 was released few days ago, I've changed it's version in tox config to PyPI one. There are still few # type: ignores; the functools.partial one needs python/typing#11 (which won't land too soon) and AbstractEventLoop.create_connection, which already has PR ready in typeshed repo (and will hopefully get included in next mypy release).

numberoverzero commented 7 years ago

Thanks very much! If you haven't yet, feel free to add yourself to the Contributors section in the README.