Update client.py

[spacemanspiff2007]

Fixes #268

[spacemanspiff2007]

I tried to add it but I am not fully sure if everything is in the right place ... . I'm not sure if allowing None for client.messages is a good idea. Maybe raising an exception instead of setting it to None would be better ...

[empicano]

Thanks for another PR from you! 🎸

I'm not sure if allowing None for client.messages is a good idea.

I see this now as well. It also messes up the typing (I'm no pro with typing in Python, so could be that we can do this differently as well). The underlying problem is that generators in Python are invalidated once they throw an exception. It's possible to hack around this, but that will only be confusing in the end.

I think the way forward could be to rename Client._messages() to Client.messages() and use it like:

async with aiomqtt.Client("test.mosquitto.org") as client:
    await client.subscribe("temperature/#")
    async for message in client.messages():
        print(message.payload)

Which is a breaking change. That's on me for not catching this issue when I implemented v2.0.0, sorry about that!

I think the best way for now is to use Client._messages(), what do you think? @frederikaalund? And I'll keep the breaking change in mind for v3.0.0.

[spacemanspiff2007]

For me this is not an issue because I'm using a workaround.

However I think Client.messages() makes more sense than Client.messages. The only graceful migration I see is to rename the Client.messages altogether.

I don't think using Client._messages() is a good idea and I think this fix definitely deserves a version bump and should be done asap. If properly documented in the release notes this should not be an issue.

[vvanglro]

It seems that adding reusable features pits us.

This means that in the future, when initializing the Client class, all the class properties that need to be instantiated need to be taken care of to make sure that reusable does not cause problems.

[spacemanspiff2007]

Imho .messages() makes more sense because it aligns e.g. with dict.keys(). If you prefer .messages then a proper type hint should be supplied. That's difficult because you can fall back to None which means every user has to check for None before accessing .messages. If in __init__ only the hint is provided the client will raise an AttributeError when accessing .messages outside the context manager which is strange, too.

[empicano]

I believe that this is fixed with #312. I only just now saw the additional commit with the migration suggestion you made, @property was a good idea, sorry about the oversight! Please let me know if there's anything left unsolved.

Thank you for the work you put into this PR nonetheless!

Imho .messages() makes more sense because it aligns e.g. with dict.keys()

That's a really good example! Like dict.keys(), .messages() returns a dynamic view (in our case of the message queue) so that fits well, I agree 👍