Version 2.0

[empicano]

This PR should implement some changes discussed in #226:

• Remove deprecated connect/disconnect methods
• Remove deprecated filtered_messages and unfiltered_messages methods
• Switch to client-wide queue
• Rename id to identifier
• Remove deprecated message_retry_set Client argument
• Update README and documentation
• Add migration guide

I'm very fond of the idea to switch out paho with rumqtt, but I think that's better suited for a 3.0, otherwise this gets too big. I have too many ideas and too little time 😄

Feedback is welcome 😋

[codecov]

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (60d96e1) 84.5% compared to head (b71e2e0) 85.1%.

Files	Patch %	Lines
aiomqtt/client.py	77.7%	6 Missing and 4 partials ⚠️
aiomqtt/message.py	95.2%	1 Missing ⚠️
aiomqtt/topic.py	98.0%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #262     +/-   ##
=======================================
+ Coverage   84.5%   85.1%   +0.6%     
=======================================
  Files          4       6      +2     
  Lines        486     425     -61     
  Branches      92      83      -9     
=======================================
- Hits         411     362     -49     
+ Misses        47      37     -10     
+ Partials      28      26      -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JonathanPlasse]

Awesome work.
Will take a look this weekend.

[frederikaalund]

Really great to see all the effort put into this 😄 👍 I'll review this weekend as well. :)

[frederikaalund]

First of all, excellent work on this PR. 👍 It's clearly the result of a thought through process.

Overall, I like the idea of client-wide queue. Seeing the resulting (much simpler) code it looks like a much better design. I also like the idea that our users can build a "distributor" on top of it. It is a breaking change and there will be push back from our 1.x users (that's always the case when you break something). Still, I think we should move forward with it. We gain a lot in terms of maintainability (the client is much simpler now).

I read through all the code but frankly I don't have much to add. Just two nit-picky comments. That's a testament to the quality of your design and implementation. Again, excellent job on that front! 😄

I also like the idea to push all the other changes that we have discussed during the past year to a 3.0 release. This way, we can land the 2.0 release right away.

Lastly, it's great to see that you touch the entire code base: Client, documentation, and tests. It's very well put together and an exemplary PR. 👍

[JonathanPlasse]

Great work! Everything seems good to me.

[empicano]

Thank you both for your valuable comments! 😊

There's another thing that I was not 100% sure about, maybe you have some ideas on that: I currently implemented the messages generator as Client.messages attribute. We could also implement it as Client.messages() method by accessing the current _messages() directly rather than calling and assigning it inside __init__.

I'm not sure if this is just a question of style or if there are other reasons for or against. I opted for Client.messages because that makes it feel more intuitive (to me) that only one generator can be used concurrently.

What do you think?

[JonathanPlasse]

Client.messages makes sense for me.

[frederikaalund]

Thank you both for your valuable comments! 😊

There's another thing that I was not 100% sure about, maybe you have some ideas on that: I currently implemented the messages generator as Client.messages attribute. We could also implement it as Client.messages() method by accessing the current _messages() directly rather than calling and assigning it inside __init__.

I'm not sure if this is just a question of style or if there are other reasons for or against. I opted for Client.messages because that makes it feel more intuitive (to me) that only one generator can be used concurrently.

What do you think?

I think it is easiest for our users to have Client.messages (as a property). However, this approach comes with some caveats. If two consumers iterate through Client.message concurrently then:

• Each gets unique messages (there is no copying)
• There is (to my knowledge) no fairness guarantees. That is, one consumer may get all the messages.
• The routing model is essentially anycast.
• If our end users want a different routing model (e.g., broadcast) then they have to build that themselves on top of aiomqtt.

The above may be surprising to our users. The easiest "work around" is just to write it very explicitly in the documentation. I don't think we should try to solve this routing problem. That's too difficult.

[empicano]

I agree that we shouldn't implement different routing models, that would probably build back the same complexity we had before 👍 Currently, something like this:

import asyncio
import aiomqtt


async def handle(client):
    async for message in client.messages:
        print(message.payload)


async def main():
    client = aiomqtt.Client("test.mosquitto.org")
    async with asyncio.TaskGroup() as tg:
        async with client:
            tg.create_task(handle(client))
            tg.create_task(handle(client))


asyncio.run(main())

fails with RuntimeError: anext(): asynchronous generator is already running; I think that's a good default.

Thanks for the great feedback! I'll merge and release this then 😊