Add type hints

[flyte]

This adds type hints to .pyi files to avoid changing/cluttering up the source files.

I've gone through the paho code to verify the types used, but not tested it beyond running mypy --strict asyncio_mqtt.

[frederikaalund]

Hi Ellis. Thanks for your pull request. Let me have a look. :)

Great to see some typing. 👍 I wanted to add this at one point or another myself. As for the types themselves, I have no objections. It looks good.

I'm wondering if we should simply inline the types instead of using stub files. Minimum Python version is 3.7 so that isn't a barrier. Likewise, we can use the built-in typing module.

What do you think? I know that you mention "avoid changing/cluttering" as the main reason. Are there other reasons for this preference?

Again, thanks for this pull request.

[flyte]

I actually prefer doing it inline, as you don't need to keep two files in sync. I can move them over when I get a moment. I might also add hints for the private methods too, since it's helpful for development on this project as well as for projects using this client.

[frederikaalund]

I actually prefer doing it inline, as you don't need to keep two files in sync.

I agree. :)

I can move them over when I get a moment. I might also add hints for the private methods too, since it's helpful for development on this project as well as for projects using this client.

I would really appreciate that, thank you. 👍

[frederikaalund]

I can merge this right away. Let me know if you prefer that, or, if you prefer to change it into inline typings first.

[flyte]

I'll change into inline tomorrow hopefully, so don't bother merging just yet. Cheers

[flyte]

With a large change like this, it's super helpful to use auto-formatting (I can't imagine working without it any more). It may be a separate issue to raise, but what do you feel about using https://github.com/psf/black to format the code?

I'll make a draft PR in a minute to show you what the changes would be when using black to format the codebase.

[frederikaalund]

It's a great idea. 👍 See my reply in #43 for details.

I feel that we align on all these recent issues. Do you want to co-maintain asyncio-mqtt with me? This way, you can freely make changes/improvements without delay. It's still the early days (pre v1.0.0), so there's plenty of opportunity to put a personal touch on the project (give you a feeling of ownership). :)

[flyte]

I feel that we align on all these recent issues

Yeah, it's slightly uncanny!

Do you want to co-maintain asyncio-mqtt with me?

Sure! Thanks 😁 . I was part-way through writing my own wrapper around paho-mqtt before I found this one. I thought https://github.com/mossblaser/aiomqtt was the only one, and I thought (hoped, ignorantly) that I might be able to make something that integrates paho more closely with the asyncio event loop. Then I saw that you had beaten me to it!

I can't promise to always be engaged with the project; I tend to dip in and out of things a bit, but I'd be happy to help push it along.

[frederikaalund]

You're now officially an asyncio-mqtt maintainer. 🎉 Welcome on board.

I had a real close look at aiomqtt (and the other alternative) as well before I created asyncio-mqtt. Great minds think alike. :))

I can't promise to always be engaged with the project; I tend to dip in and out of things a bit, but I'd be happy to help push it along.

No worries; I do the same. Put in the time that you can afford and no more.

[frederikaalund]

You can merge this PR yourself now when you see fit. 👍

[flyte]

Cool, thanks!

I wonder how you feel about enabling the new GitHub Discussions feature? I've just done it on one of my repos and it seems like a nice way to discuss things without making a bunch of issues.

[frederikaalund]

I never tried GitHub Discussions but let's give it a go. :) I enabled it.

[flyte]

I've added inline type hints throughout and split up the _pending_calls list into:

self._pending_subscribes: Dict[int, "asyncio.Future[int]"] = {}
self._pending_unsubscribes: Dict[int, asyncio.Event] = {}
self._pending_publishes: Dict[int, asyncio.Event] = {}

so that we don't have Dicts with different types of Futures and Events in. The correct dict gets passed in to _pending_call() so that the Future/Event can be added and removed.

I added a property that returns a set of mids that have pending calls, so that we're still able to get a count of them and check if the mid's already got a pending call:

    @property
    def _pending_calls(self) -> Set[int]:
        """
        Return a set of all message IDs with pending calls.
        """
        mids: Set[int] = set()
        mids.update(self._pending_subscribes.keys())
        mids.update(self._pending_unsubscribes.keys())
        mids.update(self._pending_publishes.keys())
        return mids

_cb_and_generator() was a bit tricky to follow through, but I think I managed to end up with the right types for everything!

I know I can merge myself, but I'll leave it for you to review beforehand, in case there's something you want to discuss.

[frederikaalund]

Looks good to me. 👍 I only have some minor comments here and there. Nothing of real importance. Have a look if you want to.

You can merge this PR in when you feel like it. :)

[frederikaalund]

I personally prefer the shorter pending_dict.pop(mid, None) compared to the four lines of try..except. I guess that try..except is more explicit, though. It'll do. 👍

[flyte]

Right, so that's because the values in pending_dict can't be None, so the default causes a type error. Bit silly since we're not assigning it to anything, but there we go. I had to change it to this, although I agree using pop is nicer.

[frederikaalund]

Alternatively, use from __future__ import annotations. Just check if that's Python 3.7 compatible beforehand. I can't remember. :)

[flyte]

Yeah, I do tend to, but I've been writing 3.6+ code lately. What about this project is 3.7+ only? I ask because I've used it in my 3.6+ project 🤦

[frederikaalund]

I just checked to be sure: from __future__ import annotations seems to be a Python 3.7 thing. :)

What about this project is 3.7+ only?

I think asyncio-mqtt (mostly) Python 3.6 compatible thanks to the efforts of pallas. I haven't kept track lately, though, so I might have broken 3.6 compatibility somewhere. 😅

[frederikaalund]

Thanks for the huge effort on this one. 👍 👍

Really nice to have types throughout the API (and internally). That'll make maintenance way easier going forward. :)